[asterisk-dev] [Code Review] SIP URI comparison test (plus some bug fixes)

[Matthew Nicholson]

> On 2010-07-19 17:53:43, David Vossel wrote:
> > /trunk/channels/sip/reqresp_parser.c, lines 2027-2032
> > <https://reviewboard.asterisk.org/r/792/diff/2/?file=11653#file11653line2027>
> >
> >     If there is no scheme in the uri (which is not valid but happens occasionally) this will crash on the strcmp.  I noticed that there was a comment in the old code that said the scheme must be present because parse_request checks it.  I'm guessing this is still the case, but this still looks scary to me.
> 
> Mark Michelson wrote:
>     The reason I deleted the old comment was that sip_uri_cmp() was only used after parse_request() had been called. As soon as I started adding test cases, that was no longer true. Of course this does mean that I need to be more careful about things.
>     
>     Now, this does bring up an interesting point. Should the sip_uri_cmp() function have the onus of having to check for URI correctness, or should it be assumed that by the time sip_uri_cmp() is called, we know for sure that the URIs are valid SIP or SIPS URIs?
> 
> Matthew Nicholson wrote:
>     I can make this crash by testing a uris in the following form:
>     
>     { "sip", "sip", ... }
> 
> David Vossel wrote:
>     I use this function in my request to dialog matching now as well, so there is no guarantee this will be safe in the future.
> 
> Mark Michelson wrote:
>     Actually, I just realized that David's original comment isn't entirely accurate. If there's no URI scheme then this shouldn't crash on the strcmp(). If a ":" is not encountered in the URI, then strsep will return the entire string and set either uri1 or uri2 NULL. The crash would occur later in the function when trying to use strchr on uri1 and uri2. Matt, when you were causing your crashes, was this where you were seeing the crash occur?

I think so, but I did not look at the core dump.  Simply testing a string without a ':' was not enough to make it crash.  I think it has to be either "sip" or "sips".


- Matthew


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


On 2010-07-20 11:13:30, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/792/
> -----------------------------------------------------------
> 
> (Updated 2010-07-20 11:13:30)
> 
> 
> Review request for Asterisk Developers and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> Issue 17662 was opened by oej since he discovered that using a string comparison for domains was not a viable solution since IPv6 addresses may be represented in many different ways, all of which are considered to be the same address. The patch attached on that issue was my initial attempt at solving the issue.
> 
> While I was dabbling in the area, I felt it would be a good idea to add a SIP URI comparison internal test to be sure that all the logic was correct. What I found was that in many regards, the comparison of URI parameters was not working as it should be. That being the case, I largely rewrote the logic of sip_uri_params_cmp() to work correctly.
> 
> All SIP URI comparison code has been moved to channels/sip/reqresp_parser.c, including the new test. If you have good ideas for test cases that I haven't thought of yet, please feel free to add comments to the review request.
> 
> Also of note, the bugs I found in sip_uri_params_cmp() affect 1.4 and 1.6.2 as well. My plan is to backport the bugfixes to those branches as well once I get a "Ship it!" on this review.
> 
> 
> This addresses bug 17662.
>     https://issues.asterisk.org/view.php?id=17662
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 277873 
>   /trunk/channels/sip/include/reqresp_parser.h 277813 
>   /trunk/channels/sip/reqresp_parser.c 277813 
> 
> Diff: https://reviewboard.asterisk.org/r/792/diff
> 
> 
> Testing
> -------
> 
> The new set of tests passes.
> 
> 
> Thanks,
> 
> Mark
> 
>