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

[Mark Michelson]

> On 2010-07-20 08:53:00, Matthew Nicholson wrote:
> > /trunk/channels/sip/reqresp_parser.c, line 2155
> > <https://reviewboard.asterisk.org/r/792/diff/2/?file=11653#file11653line2155>
> >
> >     You should probably add a test to match an ip against a hostname where the hostname matches the IP:
> >     
> >     { "sip:bob at localhost", "sip:bob at 127.0.0.1", URI_CMP_NOMATCH }
> >     
> >     I'm not sure if that should match or not.
> >     
> >     Also I don't know if alternative representations of ipv4 addresses are allowed by sip, but you may want to try matching other ipv4 address representations such as decimal and hexadecimal addresses.
> >     
> >     You may also want to test matching parameters that have a different order on each addresss.
> >     
> >     { "sip:bob at example.com;param2=value2;param1=value1", "sip:bob at example.com;param1=value1;param2=value2", URI_CMP_MATCH }
> >     
> >     I don't know if a malformed address would ever get passed to this function, but you may want to test with malformed addresses too.
> >     
> >     ":bob at example.com"
> >     "bob at example@com"
> >     "bob:example:com"

I'll add the hostname and same logical IP address test. RFC 3261 Sec. 19.1.4 specifically mentions such a situation as not being a match.

I can look at the SIP ABNF to see if it has any specifics about how IPv4 addresses must be formatted. If there is no restriction, I can put another test in to be sure things work.

I'll also add the "parameters match but are in a different order" test.

As far as malformed addresses go, I'll fall back to what I just wrote on David Vossel's comments. Should a URI comparison function be burdened with also being sure the URIs are valid, or should this be left to a separate function? Should sip_uri_cmp() have the precondition that we already know the URI is not malformed?


- Mark


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


On 2010-07-19 15:27:08, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/792/
> -----------------------------------------------------------
> 
> (Updated 2010-07-19 15:27:08)
> 
> 
> 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
> 
>