[asterisk-dev] [Code Review]: Fix natdetected flag being set when VIA doesn't include port in address

[elguero]

> On Nov. 26, 2012, 3:50 a.m., wdoekes wrote:
> > branches/11/channels/chan_sip.c, lines 17754-17764
> > <https://reviewboard.asterisk.org/r/2206/diff/1/?file=32226#file32226line17754>
> >
> >     For the sake of readable code, while you're in the neighourhood, I'd move the last bit into an else:
> >     
> >     if (ast_sockaddr_resolve_first(&tmp, c, 0)) {
> >        ..WARN..
> >        port = STANDARD_SIP_PORT;
> >     } else if (!(port = ast_sockaddr_port(&tmp))) {
> >        ...
> >     }
> >     
> >     It already works like that, but this ensures it stays working if someone changes code around somewhere else.
> 
> Mark Michelson wrote:
>     I agree with Walter here. It seems a bit odd to check tmp's port if an address could not be resolved.
> 
> elguero wrote:
>     I am trying to figure out what it is that I am not getting.  Please be patient with me :)
>     
>     We are setting the socket address (tmp) by using ast_sockaddr_resolve_first.  "c" holds the address.  So lets say c == "216.115.69.144".  We go into ast_sockaddr_resolve_first.  "c" is a valid ip address.  We do not fail to resolve the address.  "tmp" is set to "216.115.69.144:0".
>     
>     What I am trying to fix here is the fact that "tmp" was set to "216.115.69.144:0".  When a port is not specified in the host address of a VIA header, we should default to 5060, right?
>     
>     So, the problem is that later on in the code we do, ast_sockaddr_cmp(&tmp, &p->recv) in order to turn on the auto nat stuff.  tmp == "216.115.69.144:0" and p->recv == "216.115.69.144:5060".  This causes the code to think that the addresses are different and therefore NAT should be turned on.
>     
>     Therefore, am I being asked to change the code above that to fix up the case when an address is not resolved, which wasn't the focus of this fix but rather would be another issue that I should tackle?  Also, that would affect 1.8+.  Whereas right now, this is only tackling an issue present in 11.  I do agree with Mark that it seems odd to check this port if the address is not resolved.  I think that might be another issue because we continue to work with the socket address "tmp" later on which is probably wrong.  Please help me understand what I am not quite getting.
>     
>     Thanks
> 
> wdoekes wrote:
>     > [I am] being asked to change the code above [which is] another issue.
>     
>     Partly. When you're touching the code, you should always try to improve readability.
>     
>     (1) Unless other code changes, the behaviour stays the same (*). That means that you can safely alter this.
>     
>     (2) If code changes -- someone removes the 0-initialization of tmp OR sockaddr_resolve_first edits tmp even though it returns failure -- then you have broken code. By fixing the code now, you're preventing errors down the line. And you're making code more readable.
>     
>     (*) In fact: (in the lookup-fail-case) you're changing a comparison of "p->recv" against "0.0.0.0:0" to a comparison of "p->recv" against "0.0.0.0:5060". Not that that matters, since "p->recv" will never hold "0.0.0.0:*". By swapping the code around like I suggest: you're (a) fixing the bug, (b) keeping the original behaviour and (c) improving readability/maintainability.

Walter, thanks.  I don't know why I was not parsing things properly yesterday.  Today, it is much clearer.

Updated patch on the way.


- elguero


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


On Nov. 25, 2012, 10:37 p.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2206/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2012, 10:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> While troubleshooting another issue, came across this message in the debug log:
> [2012-11-25 19:07:25] DEBUG[13375][C-00000007]: chan_sip.c:17910 check_via: NAT detected for 216.115.69.144:0 / 216.115.69.144:5060
> Sending to 216.115.69.144:5060 (NAT)
> 
> Upon looking at the code in check_via, I discovered that if the port is not set on the address in the VIA header, the default SIP port is not being set on struct ast_sockaddr tmp which is used to check if the requesting UA might be behind NAT.
> 
> 
> This addresses bug ASTERISK-20724.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20724
> 
> 
> Diffs
> -----
> 
>   branches/11/channels/chan_sip.c 376416 
> 
> Diff: https://reviewboard.asterisk.org/r/2206/diff
> 
> 
> Testing
> -------
> 
> Local dev machine.
> 
> 
> Thanks,
> 
> elguero
> 
>

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