[asterisk-dev] [Code Review]: chan_sip check_via does a hostname lookup but discards results anyway

[elguero]

> On March 18, 2013, 12:44 p.m., elguero wrote:
> > /branches/1.8/channels/chan_sip.c, lines 16478-16480
> > <https://reviewboard.asterisk.org/r/2400/diff/1/?file=34745#file34745line16478>
> >
> >     Curious if we could use what is in asterisk code for checking if ports are valid, although it is pretty clear as it is.
> >     
> >     You could call ast_sockaddr_split_hostport with the flag PARSE_PORT_REQUIRED which would fail immediately without any extra checks needed.
> >     
> >       if (!ast_sockaddr_split_hostport(tmp, &tmphost, &tmpport, PARSE_PORT_REQUIRED)) {
> >           ast_log(LOG_WARNING, "Could not split host/port for '%s'\n", c);
> >       }
> >     
> >     Upon success of the above, you could also check that the port returned is valid by using ast_parse_arg().
> >     
> >       else if (ast_parse_arg(&tmpport, PARSE_UINT32|PARSE_IN_RANGE, &port, 1024, 65535)) {
> >         ast_log(LOG_WARNING, "Invalid port number, '%s', in VIA header\n", tmpport);
> >         port = STANDARD_SIP_PORT;
> >       }
> >     
> >     In other parts of the code, when we check for valid ports we are using the range 1024-65535, not 0-65535.

I meant 1-65535.


- elguero


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


On March 18, 2013, 9:16 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2400/
> -----------------------------------------------------------
> 
> (Updated March 18, 2013, 9:16 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The sent-by-addr address in the Via is supposed to get ignored and the sent-by-port is only used if we're not doing force_rport.
> 
> Nevertheless did check_via do hostname lookups even though it only wanted to get at the port.
> 
> I've changed the code around so it uses ast_sockaddr_split_hostport() instead of ast_sockaddr_resolve_first(). This also removes an ERROR notice which isn't worthy of such severity.
> 
> 
> Before, when someone sent something resolvable in the Via:
> 
>   chan_sip did an DNS A lookup, but ignored the results.
> 
> Now:
> 
>   chan_sip just looks at the port.
> 
> 
> Before, when someone sent a bad via header:
> 
>   ERROR[29769]: netsock2.c:269 ast_sockaddr_resolve: getaddrinfo("127.0.1.1", "5061branch=z9hG4bK-11063-1-0", ...): Servname not supported for ai_socktype
>   WARNING[29769]: chan_sip.c:16921 check_via: Could not resolve socket address for '127.0.1.1:5061branch=z9hG4bK-11063-1-0'
> 
> Now:
> 
>   WARNING[10229]: chan_sip.c:16928 check_via: Could not split host/port for '127.0.1.1:5061branch=z9hG4bK-11049-1-0'
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 383308 
> 
> Diff: https://reviewboard.asterisk.org/r/2400/diff
> 
> 
> Testing
> -------
> 
> 57 SIP tests ran successfully with and without the patch.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130318/91a33161/attachment.htm>