[asterisk-dev] [Code Review] IPv6 in Asterisk

[Simon Perreault]

On 2010-07-08 11:15, Mark Michelson wrote:
> The fix you committed in r274684 is not enough to fix this problem.
> The reason is that most of the time the ast_sockaddr passed into
> these functions was created on the stack with no initial values set.

Yes.

> This means that ast_sockaddr_isnull() will almost always return
> false,

No. The calls to ast_sockaddr_isnull() are applied on
instance->local_address and instance->remote_address, not the
ast_sockaddr parameter.

> thus causing the use of uninitialized values that I had
> complained of earlier.

Looking at it again, I think we need to revert the change and leave it
the way it was. Here's the proposed code:

> int ast_rtp_instance_get_local_address(struct ast_rtp_instance *instance,
>                 struct ast_sockaddr *address)
> {
>         if (ast_sockaddr_cmp(address, &instance->local_address) != 0) {
>                 ast_sockaddr_copy(address, &instance->local_address);
>                 return 1;
>         }
> 
>         return 0;
> }
> 
> int ast_rtp_instance_get_remote_address(struct ast_rtp_instance *instance,
>                 struct ast_sockaddr *address)
> {
>         if (ast_sockaddr_cmp(address, &instance->remote_address) != 0) {
>                 ast_sockaddr_copy(address, &instance->remote_address);
>                 return 1;
>         }
> 
>         return 0;
> }

When address is null, it will work as intended. If address is full of
uninitialized garbage, it will also work as intended even though
valgrind will complain. But that is not different from what was in trunk
and is completely unrelated to IPv6.

> The only way I know to get around this is to
> find all cases where you are creating an ast_sockaddr structure on
> the stack and then either call ast_sockaddr_setnull() on the
> structure prior to calling these RTP functions, or initialize the
> ast_sockaddr structures to be all 0s at the time of their creation.

I agree. But this is unrelated to IPv6 and I'd prefer if we kept this as
a separate issue.

If you agree, can you please confirm that the proposed code above is
valid? I would then commit it and produce a new patch for the review board.


> There's still a problem here after r274682. You still have the second
> parameter of the memcpy as &sin. It should just be sin.

You're right. Fixed in r274766.

>> Simon Perreault wrote: This is intentional. __ast_http_load() uses
>> address families as a flag. It sets them only when the sockets are
>> enabled in the configuration. We wanted to preserve the overall
>> behaviour.
> 
> Hm, I see now. It's still possible to avoid the warning messages
> though. You can set the address family to AF_INET here initially.
> Then, lower in the function, there is an if (enabled) check. Add an
> else case that clears the address families.
> 
> Yes, I know that this doesn't actually cause any real problems other
> than warning messages, but the average Asterisk user will become
> alarmed by such warning messages and we'll likely get many e-mails
> and issue reports if they are present.

Agreed. Fixed in r274767.

Simon
-- 
NAT64/DNS64 open-source --> http://ecdysis.viagenie.ca
STUN/TURN server        --> http://numb.viagenie.ca
vCard 4.0               --> http://www.vcarddav.org