[asterisk-dev] [Code Review] Don't include IPv6 link-local scope-ids in SIP messages

[Simon Perreault]

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


This patch is important and well executed. I especially like the new tests file.

There is one design issue I'm not too sure about: the need for sip_sanitized_host(). Would it be possible to use *_remote() when creating p->tohost and r->hostname instead? Is there any reason at all why we would want the scope id in there? The only reason I can see is if we use p->tohost or r->hostname to create a socket. In that case the patch is fine as it stands, otherwise it would be better to do away with sip_sanitized_host().


/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1278/#comment7542>

    Couldn't p->tohost contain a port number? If so, then PARSE_PORT_FORBID should be replaced with 0.
    
    In fact, PARSE_PORT_FORBID should be used when there's a hard reason why a port is unacceptable. If there is one, please add it in comments.



/branches/1.8/main/netsock2.c
<https://reviewboard.asterisk.org/r/1278/#comment7540>

    Here it would be better to leave the case labels as they were and instead do:
    
    switch (format & (AST_SOCKADDR_STR_ADDR | AST_SOCKADDR_STR_PORT | AST_SOCKADDR_STR_BRACKETS)) {
    
    Feel free to #define something like AST_SOCKADDR_STR_FORMAT_MASK instead of hard-coding the three bits as I did above.



/branches/1.8/main/netsock2.c
<https://reviewboard.asterisk.org/r/1278/#comment7541>

    The "&& addr->len" part is unnecessary because ast_sockaddr_is_ipv6() already checks it.


- Simon


On 2011-06-22 00:28:19, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1278/
> -----------------------------------------------------------
> 
> (Updated 2011-06-22 00:28:19)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> chan_sip incorrectly would send IPv6 link-local addresses with the scope-id in SIP messages. This is usually an interface name on the local machine and means nothing to the remote host and should not be sent.
> 
> The original patch submitted on the issue just stripped the '%' and everything after it from the host before doing any kind of stringification. I was uncomfortable with making a global change to how these functions operated and instead added new _remote versions of the stringification functions and had chan_sip use them when the results would end up in a SIP message. The non-remote versions of the functions are still used in things like debug messages and anywhere we might be using the information to make an outbound connection since the scope-id is necessary information in those cases.
> 
> Since registrations were the focus of the bug report, I also noticed that the URI, From, and To of the registration also included the scope-id and didn't use the stringification functions, so I added sip_sanitized_host() which would take a host and if it happened to be a link-local ipv6 address, would "do the right thing".
> 
> I also added some netsock2 parsing tests.
> 
> I am not an IPv6 guru, so I would appreciate someone who is verifying this patch is a good idea.
> 
> 
> This addresses bug ASTERISK-17711.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17711
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 324299 
>   /branches/1.8/include/asterisk/netsock2.h 324299 
>   /branches/1.8/main/netsock2.c 324299 
>   /branches/1.8/tests/test_netsock2.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1278/diff
> 
> 
> Testing
> -------
> 
> I tested registering two asterisk boxes to each other using link-local ipv6 addresses. They failed before the patch and succeeded after.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110622/c3aafd4b/attachment-0001.htm>