[asterisk-dev] [Code Review] Introduce function for parsing ABNF structure {name-andor-addr = name-addr / addr-spec} in sip messages

[Nick Lewis]

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > This is my first pass through this review request. Even though there are many areas that need improvement, I have to say that this is an excellent addition to the code, and you get bonus points from me for including unit tests along with the code changes.
> > 
> > In addition to what I marked below, please also make an effort when posting your next diff to remove as much trailing whitespace as you can from the diff so that there aren't as many red blotches on the review.
> > 
> > Thanks!
> 
> Nick Lewis wrote:
>     Thank you very much for all your comments
>     
>     Are there any formatting tools that you would recommend for whitespace, tabbing etc?
> 
> Mark Michelson wrote:
>     I don't have any sort of recommendation there. I use vim as my editor, and its default behavior is to indent using a tab instead of X number of spaces. doc/CODING-GUIDELINES has a sample command that you can use for the 'indent' utility that may be helpful for you.

Thanks. Looking again I see that the problem was a compatibility issue between the ssh client on my pc and the text editor on the asterisk development unit which led to copy/pastes replacing tabs with spaces 


> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 162-205
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line162>
> >
> >     It is somewhat common for certain parameters to not contain an "=" sign, particularly the "lr" parameter. If I have read this loop correctly, we will
> >     
> >     1) Not properly parse any parameter that does not contain an "="
> >     2) Not parse any further parameters if we come across one that does not contain an "="
> >     
> >     I think this loop needs to be rewritten in a manner that does not depend on an "=" being present for all parameters.
> 
> Nick Lewis wrote:
>     Thanks for identifying this issue. I had not realised that "lr" was a special case. I will try again
> 
> Mark Michelson wrote:
>     Yeah, "lr" is kind of a strange thing to deal with. There have been several threads on the sip-implementors mailing list that deal with ways this is used. Some UAs will simply just place "lr" in the URI, while others will do something like "lr=on". From what I have gleaned, it's the presence or absence of this header that is meaningful rather than its value. With that in mind, it may be a good idea to change the uriparams structure so that the "lr" field is an int indicating whether the parameter is present rather than a string that stores the parameter's value.

ok


> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/include/reqresp_parser.h, line 54
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8526#file8526line54>
> >
> >     I have a suggestion for this function.
> >     
> >     Seeing the unruly number of parameters to this function, I think a better approach would be to wrap all the output parameters in a struct instead of having each individual one returned.
> >     
> >     Also, "remainder" is spelled incorrectly here.
> 
> Mark Michelson wrote:
>     In case the sheer number of parameters wasn't enough of a reason for you to make this change, there are also some other reasons, mostly having to do with the fact that all output pointers can be specified NULL. There are several cases where you can end up with less-than-desirable results.
>     
>     For instance let's take the following URI into account:
>     
>     sip:user at host:5060;param1=value1?header1=value2
>     
>     Let's look at a particular combination that has some unexpected results:
>     If you call parse_uri_full with headers == NULL and uriparams == NULL, but remainder (or residue as it's called in the function definition) non-NULL, then on returning, residue will be set to "host" and port will be set to "5060;param1=value1?header1=value2"
>     
>     If you change the function prototype to take a struct into which all the output parameters are placed, then there's no worry about the various combinations of NULL and non-NULL pointers to have to deal with.
> 
> Nick Lewis wrote:
>     I think you have described a bug with residue="host". Would my change at line 133 of the .c fix this?
>     
>     I was copying two aspects of the original parse_uri function here. The first was the separate variables and the second was the ability to pass NULL if one did not want to terminate the string at the detected uri component. The former made it a bit easier to reimplement the parse_uri function and, although I have no idea why one would want to do it, the latter is retained for backward compatibility.
>     
>     If the requirement for passing NULL **char pointers can be dropped then I will change to a struct
>
> 
> Mark Michelson wrote:
>     > "I think you have described a bug with residue="host". Would my change at line 133 of the .c fix this?"
>     
>     No, that will not help here. On a second look, I made a small mistake in my analysis, but the results are still less than ideal. When the port is parsed, you set "uri" to point to the port. Then, if params and headers are both NULL (and by that, I mean the function parameters with those names), then there is no further parsing done. The result is that the port is not properly null-terminated, so that it is set to "5060;param1=value1?header1=value2." In addition, since we never updated the uri pointer any further, residue points to the same string as port, meaning it also is set to "5060;param1=value1?header1=value2."
>     
>

I had intended that residue would be "" in this example but as you point out there is a higher level bug with the returned port. 

In the original parse_uri I saw the non-terminating behaviour with "host:port" when **port was NULL and assumed it was the same with "port;transport=tcp" when **transport was NULL. I now see that port is terminated even when **transport is NULL. I will make the code for parameters, headers and residue insert all terminators even when they are not asked to return anything. Will this fully address the issue?


- Nick


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/549/#review1659
-----------------------------------------------------------


On 2010-03-11 03:12:54, Nick Lewis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/549/
> -----------------------------------------------------------
> 
> (Updated 2010-03-11 03:12:54)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> Many sip headers in many sip methods contain the ABNF structure
>  name-andor-addr = name-addr / addr-spec
> Examples include the to-header, from-header, contact-header, replyto-header, referto-header, referredby-header, and passertedid-header
> 
> At the moment chan_sip.c makes various different attempts to parse this name-andor-addr structure for each header type and for each sip method with sometimes limited degrees of success.
> 
> I here introduce a dedicated function for parsing the name-andor-addr ABNF structure that can be used irrespective of the specific method or header that contains the structure
> 
> (The function is also suited to parsing the name-addr ABNF structure without the addr-spec option. Examples in chan_sip.c include the recordroute-header, route-header, remotepartyid-header and diversion-header.)
> 
> 
> This addresses bug 16708.
>     https://issues.asterisk.org/view.php?id=16708
> 
> 
> Diffs
> -----
> 
>   trunk/channels/sip/include/reqresp_parser.h 249060 
>   trunk/channels/sip/include/sip.h 249060 
>   trunk/channels/sip/reqresp_parser.c 249060 
> 
> Diff: https://reviewboard.asterisk.org/r/549/diff
> 
> 
> Testing
> -------
> 
> Tested using the corresponding unit test function
> 
> 
> Thanks,
> 
> Nick
> 
>