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

[Mark Michelson]

> On 2009-01-19 14:35:46, Kevin Fleming wrote:
> >

Changes have already been made in the branch and an updated diff will be uploaded shortly


> On 2009-01-19 14:35:46, Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, lines 7117-7119
> > <http://reviewboard.digium.com/r/126/diff/1/?file=2311#file2311line7117>
> >
> >     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.

Corrected in branch both here and the place further down in the function where I made the same change.


> On 2009-01-19 14:35:46, Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, line 7167
> > <http://reviewboard.digium.com/r/126/diff/1/?file=2311#file2311line7167>
> >
> >     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.

Fixed this to be req->str->used since that will be a null byte.


> On 2009-01-19 14:35:46, Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, lines 9409-9412
> > <http://reviewboard.digium.com/r/126/diff/1/?file=2311#file2311line9409>
> >
> >     These are unnecessary, they are covered by the memcpy() of the entire structure at line 9391.

Fixed.


> On 2009-01-19 14:35:46, Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, line 20284
> > <http://reviewboard.digium.com/r/126/diff/1/?file=2311#file2311line20284>
> >
> >     Please revert this change; the code was clearer before, because it was explicitly referencing the 'first header line'.

I couldn't revert the change directly due to the change in types of req->header[0], so I just used the handy REQ_OFFSET_TO_STR macro so that things are more clear


- Mark


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


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
> 
>