[asterisk-dev] [Code Review] 2991: Fix incorrect usage of ast_get_ip involving uninitialized or ambiguously initialized ast_sockaddr

[wdoekes]

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


Just a minor nit. Michaels ship-it still stands.


/branches/12/include/asterisk/netsock2.h
<https://reviewboard.asterisk.org/r/2991/#comment19389>

    That's an odd thing to say in a (header) file that includes sys/socket.h itself. I don't see anyone undefining those values anywhere. And the rest of the code is sprinkled with [^_]AF_INET.


- wdoekes


On Nov. 2, 2013, 10:25 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2991/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2013, 10:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This started off as a fix for the failing IAX2 acl_call test in the Asterisk Test Suite.
> 
> When inspecting why that test was failing, I found that all attempts to bind to any local loopback address was failing:
> 
> [Nov  2 15:56:28] VERBOSE[15787] chan_iax2.c:   == Binding IAX2 to address 127.0.0.1:4569
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: Splitting '127.0.0.1' into...
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: ...host '127.0.0.1' and port ''.
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: Splitting '127.0.0.1' into...
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: ...host '127.0.0.1' and port ''.
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: Splitting '127.0.0.1' into...
> [Nov  2 15:56:28] DEBUG[15787] netsock2.c: ...host '127.0.0.1' and port ''.
> [Nov  2 15:56:28] ERROR[15787] netsock2.c: getaddrinfo("127.0.0.1", "(null)", ...): ai_family not supported
> [Nov  2 15:56:28] WARNING[15787] acl.c: Unable to lookup '127.0.0.1'
> [Nov  2 15:56:28] WARNING[15787] chan_iax2.c: Non-local or unbound address specified (127.0.0.1) in sourceaddress for 'local-1', reverting to default
> 
> While there's conceivably other ways for getaddrino to return EAI_FAMILY, the most common way is if AF_INET, AF_INET6, or AF_UNSPEC is not provided as the desired family. However, chan_iax2 is (more or less) doing the right thing when it calls into ast_sockaddr_resolve:
> 
> ...
> 
> if (!ast_sockaddr_resolve(&hostaddr, tmp->value, PARSE_PORT_FORBID, 0)
> 
> ...
> 
> Other than passing 0 in as the family (which should be AST_AF_UNSPEC), this should be fine. After some further debugging, it turned out that the culprit was the call to ast_get_ip. This function - which strangely enough is defined in acl.h - actually uses the family from the passed in addr object (which it will also populate when it returns!) when it eventually calls getaddrinfo. (Yes, we're using an out param as input to our function. Ew.) Some searching through the code revealed a number of places - not just in chan_iax2 - where the ast_sockaddr structure passed to ast_get_ip does not have its family initialized or its family is not specified (there's no guarantee that 0 == AF_UNSPEC on all systems).
> 
> This patch fixes those users of ast_get_ip that were not specifying the family. It also defines AST_AF_* to their actual equivalents, instead of defining them to integer values. This should make this code less brittle in case something actually doesn't use those integer values for those constants.
> 
> 
> Diffs
> -----
> 
>   /branches/12/include/asterisk/netsock2.h 402448 
>   /branches/12/include/asterisk/acl.h 402448 
>   /branches/12/channels/chan_sip.c 402448 
>   /branches/12/channels/chan_iax2.c 402448 
> 
> Diff: https://reviewboard.asterisk.org/r/2991/diff/
> 
> 
> Testing
> -------
> 
> The IAX2 acl_call test now passes on Asterisk 12.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131103/893d0333/attachment-0001.html>