[Home]

Summary:ASTERISK-16384: [patch] IPv6: get_refer_info
Reporter:Olle Johansson (oej)Labels:
Date Opened:2010-07-16 07:45:54Date Closed:2010-08-03 14:59:37
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/IPv6
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) diff
( 1) diff2
( 2) get_domain.diff
Description:14207                 domain = ptr;
14208                 if ((ptr = strchr(domain, ':'))) {      /* Remove :port */
14209                         *ptr = '\0';
14210                 }
14211


Another case where we just search for : to separate host and port
Comments:By: Mark Michelson (mmichelson) 2010-07-16 13:12:42

I initially uploaded a patch that simply removed the attempt to separate the port from the host, but I realized that the correct fix for this is a bit more involved. I'll have something soon enough though.
By: Mark Michelson (mmichelson) 2010-07-19 09:42:40

This problem is quite a bit deeper than the problem reported here. The problem is that the use of SIP domains has had a big behavior change here.

Prior to the IPv6 merge, SIP domains were purely either host names or IP addresses. Now, there are several cases in the code where domains are added with a port appended to them. One example is the auto_sip_domains section in reload_config(). Another example is in most cases where the refer_to_domain is set. In most of these cases, the port is not stripped off at all before setting the value. This value is then used as an argument for check_sip_domain(). In the case oej found, the port is attempted to be stripped off, but the method is flawed when dealing with IPv6 addresses.


By: Olle Johansson (oej) 2010-07-19 09:54:01

We should propably open another bug report for the domains, but yes, the sip domain structure should have binary IPs now, since ipv6 can have multiple notations for the same domains.
By: Olle Johansson (oej) 2010-07-19 09:54:34

I don't think the sipdomain handle a port btw...
By: Mark Michelson (mmichelson) 2010-07-19 09:59:54

Right, it doesn't make sense to me for ports to be part of the domain check.
By: Simon Perreault (sperreault) 2010-07-22 15:17:55

I think each problem should be solved individually. I don't see any big architectural problem here.

For the auto_sip_domains part in reload_config(), we need to be using ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify(), so that the port is not present.

For the broken port removal that this bug initially identified, we need to remove the port but use something more involved than strchr(). Why not use AST_NONSTANDARD_RAW_ARGS()? It will skip matching [] pairs.
By: Mark Michelson (mmichelson) 2010-07-22 15:22:41

AST_NONSTANDARD_RAW_ARGS() sounds good to me. It seems like there may be several cases in chan_sip where we'll have a string and wish to either isolate or remove the port. I think a helper function that makes use of AST_NONSTANDARD_RAW_ARGS would work well.
By: Simon Perreault (sperreault) 2010-08-02 10:33:36

I have added two diffs addressing the two separate problems.

I decided against using AST_NONSTANDARD_RAW_ARGS() because:
- Its interface makes it hard to use from here.
- It handles special characters which we don't care about.
- The additional code needed to do this correctly is minimal.
By: Mark Michelson (mmichelson) 2010-08-03 11:17:39

There is one more place that I can find that needs to be addressed, and I apologize for not pointing it out in my initial description of the problem.

In the get_domain() function, there is a strchr() statement to try to strip off the port from the domain. Much like the problem in get_refer_info(), this will not work correctly when IPv6 addresses are encountered.
By: Simon Perreault (sperreault) 2010-08-03 11:41:32

I uploaded get_domain.diff which should fix get_domain() in a manner similar to get_refer_info().

OK to apply all three diffs?
By: Mark Michelson (mmichelson) 2010-08-03 11:47:53

They all appear to fix the issue at hand, so feel free.
By: Digium Subversion (svnbot) 2010-08-03 11:52:00

Repository: asterisk
Revision: 280707

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r280707 | simon.perreault | 2010-08-03 11:51:59 -0500 (Tue, 03 Aug 2010) | 2 lines

Fixed IPv6-related SIP parsing bugs. Fixes issue ASTERISK-16384.

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280707
By: Simon Perreault (sperreault) 2010-08-03 11:52:14

Committed in r280707.
By: Digium Subversion (svnbot) 2010-08-03 13:59:56

Repository: asterisk
Revision: 280707

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r280707 | simon.perreault | 2010-08-03 11:52:01 -0500 (Tue, 03 Aug 2010) | 9 lines

Fixed IPv6-related SIP parsing bugs.

(closes issue ASTERISK-16384)
Reported by: oej
Patches:
     diff uploaded by sperreault (license 252)
     diff2 uploaded by sperreault (license 252)
     get_domain.diff uploaded by sperreault (license 252)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280707
By: Digium Subversion (svnbot) 2010-08-03 14:54:02

Repository: asterisk
Revision: 280778

U   branches/1.8/channels/chan_sip.c

------------------------------------------------------------------------
r280778 | simon.perreault | 2010-08-03 14:54:02 -0500 (Tue, 03 Aug 2010) | 9 lines

Fixed IPv6-related SIP parsing bugs.

(closes issue ASTERISK-16384)
Reported by: oej
Patches:
     diff uploaded by sperreault (license 252)
     diff2 uploaded by sperreault (license 252)
     get_domain.diff uploaded by sperreault (license 252)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280778
By: Digium Subversion (svnbot) 2010-08-03 14:59:36

Repository: asterisk
Revision: 280780

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r280780 | simon.perreault | 2010-08-03 14:59:36 -0500 (Tue, 03 Aug 2010) | 16 lines

Merged revisions 280778 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

........
 r280778 | simon.perreault | 2010-08-03 15:54:03 -0400 (Tue, 03 Aug 2010) | 9 lines
 
 Fixed IPv6-related SIP parsing bugs.
 
 (closes issue ASTERISK-16384)
 Reported by: oej
 Patches:
       diff uploaded by sperreault (license 252)
       diff2 uploaded by sperreault (license 252)
       get_domain.diff uploaded by sperreault (license 252)
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=280780