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

Simon Perreault simon.perreault at viagenie.ca
Wed Jun 30 09:21:31 CDT 2010 


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 2195-2197
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line2195>
> >
> >     These additions aren't necessary. When structs are initialized in this way, any elements not specified in the declaration are automatically zeroed out.

Removed.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 2209-2211
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line2209>
> >
> >     These are unnecessary for the same reason.

Removed.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 3248-3251
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3248>
> >
> >     Unindent the ast_debug() line by one tab. It's a bit of a nitpick, but the lines currently all look as if they are intended to belong to the else, when only the warning message does. It wouldn't hurt to add braces to the else case, too while you're here :)

Good catch! Fixed.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 4380-4393
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line4380>
> >
> >     The else case here can lead to a crash. "addrs" is not initialized to any particular value in this loop. If ast_sockaddr_resolve() returns 0, then freeing "addrs" will likely cause a crash.
> >     
> >     My suggestion is to set "addrs" to NULL at the beginning of each iteration of the loop. This way, the ast_free will always be given either a NULL pointer or a properly allocated pointer.

Yes, you are right. There was also a memory leak. Fixed.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 9065-9069
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9065>
> >
> >     This code will not function correctly if the maddr parameter contains an IPv6 address.

It will also not work if it is a host name, which RFC 3261 allows. Changed to:

strspn(maddr, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-.:[]")

both here and in check_via(). These are all the characters that are allowed according to the ABNF.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, line 10400
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10400>
> >
> >     Space after the comma.
> >     
> >     Also, you have changed the logic regarding the setting of udptldest.sin_port. Previously, it used udptlsin.sin_port, and now it is using p->ourip's port.

You're right! Fixed.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 12001-12002
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12001>
> >
> >     ast_sockaddr_port() already returns the port in host byte order, so calling ntohs() on the result is erroneous.

Actually what we need is htons(), since p->socket.port is in network byte order.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 14369-14372
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14369>
> >
> >     Is this a bug fix?

Yes! We encountered this issue along the way. rport was broken, and this is how we fixed it.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 14376-14380
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14376>
> >
> >     Similar to the other maddr comment I made, this will not work properly if the maddr parameter has an IPv6 address.

Fixed, see above.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, line 2603
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line2603>
> >
> >     I noticed this removal and did not see anything that seemed to replace what was formerly here. Can you explain?

It looks like it was removed by mistake. Re-added.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 3205-3212
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3205>
> >
> >     "want_remap" needs to be set no matter what the address scheme of "theirs" is.

This address remapping feature is used to deal with NATs and is unnecessary in IPv6. To ensure that users are notified if they try to use it, we added a warning:


       if (ast_sockaddr_is_ipv6(&theirs)) {
                if (localaddr && !ast_sockaddr_isnull(&externip)) {
                        ast_log(LOG_WARNING, "Address remapping activated in sip.conf "
                                "but we're using IPv6, which doesn't need it. Please "
                                "remove \"localnet\" and/or \"externip\" settings.\n");
                }
        } else {
                ast_sockaddr_to_sin(&theirs, &theirs_sin);
                ast_sockaddr_to_sin(us, &us_sin);

                want_remap = localaddr &&
                        !(ast_sockaddr_isnull(&externip) && stunaddr.sin_addr.s_addr) &&
                        ast_apply_ha(localaddr, &theirs_sin) == AST_SENSE_ALLOW ;
        }


- Simon


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


On 2010-06-29 10:17:24, Simon Perreault wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/743/
> -----------------------------------------------------------
> 
> (Updated 2010-06-29 10:17:24)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This is the port of Asterisk to IPv6.
> 
> 
> This addresses bug 17565.
>     https://issues.asterisk.org/view.php?id=17565
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 272684 
>   /trunk/apps/app_externalivr.c 272684 
>   /trunk/channels/chan_gtalk.c 272684 
>   /trunk/channels/chan_h323.c 272684 
>   /trunk/channels/chan_iax2.c 272684 
>   /trunk/channels/chan_jingle.c 272684 
>   /trunk/channels/chan_mgcp.c 272684 
>   /trunk/channels/chan_multicast_rtp.c 272684 
>   /trunk/channels/chan_sip.c 272684 
>   /trunk/channels/chan_skinny.c 272684 
>   /trunk/channels/chan_unistim.c 272684 
>   /trunk/channels/sip/dialplan_functions.c 272684 
>   /trunk/channels/sip/include/dialog.h 272684 
>   /trunk/channels/sip/include/globals.h 272684 
>   /trunk/channels/sip/include/reqresp_parser.h 272684 
>   /trunk/channels/sip/include/sip.h 272684 
>   /trunk/channels/sip/reqresp_parser.c 272684 
>   /trunk/include/asterisk/acl.h 272684 
>   /trunk/include/asterisk/config.h 272684 
>   /trunk/include/asterisk/dnsmgr.h 272684 
>   /trunk/include/asterisk/netsock2.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 272684 
>   /trunk/include/asterisk/tcptls.h 272684 
>   /trunk/main/acl.c 272684 
>   /trunk/main/app.c 272684 
>   /trunk/main/config.c 272684 
>   /trunk/main/dnsmgr.c 272684 
>   /trunk/main/http.c 272684 
>   /trunk/main/manager.c 272684 
>   /trunk/main/netsock2.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 272684 
>   /trunk/main/tcptls.c 272684 
>   /trunk/res/res_rtp_asterisk.c 272684 
>   /trunk/res/res_rtp_multicast.c 272684 
> 
> Diff: https://reviewboard.asterisk.org/r/743/diff
> 
> 
> Testing
> -------
> 
> See test report on the mantis issue.
> 
> 
> Thanks,
> 
> Simon
> 
>

More information about the asterisk-dev mailing list