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

[Terry Wilson]

> On 2011-06-22 07:55:11, Simon Perreault wrote:
> > 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().

First, thanks for the review! I was hoping you would see it. :-) I originally tried to do exactly what you suggest, but found we do call create_addr with r->hostname.


> On 2011-06-22 07:55:11, Simon Perreault wrote:
> > /branches/1.8/main/netsock2.c, line 105
> > <https://reviewboard.asterisk.org/r/1278/diff/1/?file=17113#file17113line105>
> >
> >     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.

Done.


> On 2011-06-22 07:55:11, Simon Perreault wrote:
> > /branches/1.8/main/netsock2.c, line 408
> > <https://reviewboard.asterisk.org/r/1278/diff/1/?file=17113#file17113line408>
> >
> >     The "&& addr->len" part is unnecessary because ast_sockaddr_is_ipv6() already checks it.

Done.


> On 2011-06-22 07:55:11, Simon Perreault wrote:
> > /branches/1.8/channels/chan_sip.c, line 12653
> > <https://reviewboard.asterisk.org/r/1278/diff/1/?file=17111#file17111line12653>
> >
> >     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.

It looks like tohost is specifically set only to the host portion of the address. hostport.host in one case, and the value of host= in another. The port would be specified by port=. DNS lookups are done on the value as well, so it doesn't seem like a port would be appropriate. I'll add a comment.


- Terry


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


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/4caafae8/attachment.htm>