[asterisk-dev] [Code Review] Convert character pointers in sip_request structure to be offsets from the beginning of the buffer

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/126/#review304
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/126/#comment683>

    You need to restore the 'int' case and the 'd' format here; on some systems, strlen() does not return a size_t, it returns an int, and those platforms don't understand the 'zd' format. The combination of casting to an int and using the 'd' format makes this more portable.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/126/#comment684>

    It has to point to an empty string (null character), in case anyone dereferences it in spite of req->lines being zero. If know that the end of req->data->str has a null character, then yes, this offset should be to that character.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/126/#comment685>

    These are unnecessary, they are covered by the memcpy() of the entire structure at line 9391.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/126/#comment686>

    Please revert this change; the code was clearer before, because it was explicitly referencing the 'first header line'.


- Kevin


On 2009-01-17 17:00:16, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/126/
> -----------------------------------------------------------
> 
> (Updated 2009-01-17 17:00:16)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Kevin Fleming.
> 
> 
> Summary
> -------
> 
> chan_sip underwent a change to use an ast_str structure inside the sip_request structure in order to allow for arbitrary
> packet sizes (to a certain extent). This also gives us the benefit of making the assumption early on that a packet will be
> small (less than 1024 chars) instead of hard-coding 4K for every packet.
> 
> One problem that arose from this conversion was that while the actual buffer in the sip_request was changed, other pointers
> in the sip_request structure were not updated appropriately. The way these previously worked was to point to an appropriate 
> spot within the static buffer so that information could be easily referenced via these pointers. The problem that has occurred 
> is that if the ast_str expands to allow more characters to be written, these pointers become invalid since they are pointing
> to "old" memory.
> 
> The rlPart1, rlPart2, header array, and line array have been changed from char * to ptrdiff_t. These represent the offsets
> from the beginning of the string. This way, the offset can be applied to the beginning of the string even if it is reallocated
> due to a size increase. To ease things a bit, I have included a macro called REQ_OFFSET_TO_STR which, if supplied with a sip_request
> pointer and one of the offsets, will do the pointer math for you and give you a string to work with.
> 
> My tests show that my logic is sound; however, I would like to have some more eyes look at this in case there are suggestions 
> for how to improve things further. I gave some serious thought to rewriting the parse_request and determine_firstline_parts
> functions from scratch since I was already modifying them, but decided instead to just add my new logic into what existed.
> 
> Also, if someone has a better idea for a name than REQ_OFFSET_TO_STR, I'm all ears.
> 
> 
> This addresses bug 14220.
>     http://bugs.digium.com/view.php?id=14220
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 168979 
> 
> Diff: http://reviewboard.digium.com/r/126/diff
> 
> 
> Testing
> -------
> 
> I tested by running calls from one SIP phone through Asterisk to another SIP phone. The calls connected just
> fine, and no errors, warnings, or other strange messages appeared on the console. I also turned on SIP debug
> just to be sure that the messages looked correct and they did. I made sure to run some tests where the size of
> SIP requests originating from Asterisk were longer than 1024 characters, too. Finally, I made the same calls
> while running Asterisk under Valgrind's memcheck tool. Valgrind made no complaints during these tests.
> 
> 
> Thanks,
> 
> Mark
> 
>