Summary:ASTERISK-12965: [patch] Cannot register with sip providers that require '@' in the username
Reporter:Navin Kumar (navkumar)Labels:
Date Opened:2008-10-25 12:44:22Date Closed:2008-12-10 10:44:33.000-0600
Versions:Frequency of
Environment:Attachments:( 0) stringfields.diff
Description:With 1.4.22, you cannot have a registration line like the following:


I believe there was an issue (ASTERISK-12786) where a patch was committed to not allowing 'reserved' characters; however, looking at the patch and the svn diff, I saw that it must have guessed the first '@' is reserved; however, this registration is perfectly valid for many sip providers.  It is how you register with particular sip hosts using a full URI instead of using a simple host name, as well as how you register through outbound proxies.  This kind of register line is absolutely necessary though several sip providers.


Link to svn diff in /branches/1.4

Link to original patch in bug report ASTERISK-12786

One should create a better way of handling malformed registration strings.  This kind of preliminary error handling which is not tested properly can lead to these scenarios.
Comments:By: Olle Johansson (oej) 2008-10-25 14:00:54

If you set the fromdomain in a peer and use that for registration, does it work?

By: Navin Kumar (navkumar) 2008-10-25 22:50:17

It doesn't seem to work for me. I tried with both SIP realtime and in the config file.  It doesn't seem to be using the fromdomain when constructing the registration.

By: James McCoy (jamessan) 2008-12-02 16:00:17.000-0600

This is actually a problem introduced by mis-use of the ast_string_field_set macro.  In chan_sip.c:transmit_register there is the following line of code:

 ast_string_field_set(p, fromdomain, ++fromdomain);

The body of the macro uses the 3rd argument (in this case "++fromdomain") in multiple places, thus causing the pointer to be incremented multiple times.

The attached stringfields.diff changes both ast_string_field_index_set and ast_string_field_index_logset to assign the data argument of the macro to a new variable (__data__) in the macro body and then use that variable instead.

By: Mark Michelson (mmichelson) 2008-12-04 16:43:04.000-0600

It appears that in my fix for ASTERISK-12786, I did not take into account that the "username" portion of the register line may contain the @ symbol.

Before any work is committed, are there any other of the "reserved" characters which may appear legally in the user portion of the register line?

jamessan's change for the string field macros seems like it should be committed too, to prevent such side effects from occurring.

By: Leif Madsen (lmadsen) 2008-12-09 07:52:44.000-0600

putnopvut: added a relation to an issue that mentions about other reserved characters in the register line. I've also added a 'has duplicate' for another issue that complains about this same issue, but I'm going to close the duplicate.

By: Mark Michelson (mmichelson) 2008-12-10 10:23:09.000-0600

This was introduced because of the fix for issue 13570. That issue probably affected maybe about 3 people total, and since the fix for it is now breaking SIP registrations for lots of people, I am going to revert that change and re-open issue 13570 instead.

By: Mark Michelson (mmichelson) 2008-12-10 10:24:14.000-0600

Oh, and I will also be committing jamessan's patch too. Don't think I've forgotten about that one :)

By: Digium Subversion (svnbot) 2008-12-10 10:24:51.000-0600

Repository: asterisk
Revision: 162663

U   branches/1.4/channels/chan_sip.c

r162663 | mmichelson | 2008-12-10 10:24:51 -0600 (Wed, 10 Dec 2008) | 11 lines

Revert fix for issue 13570. It has caused more problems than
it helped to fix.

(closes issue ASTERISK-12965)
Reported by: navkumar

(closes issue ASTERISK-13170)
Reported by: ffs



By: Digium Subversion (svnbot) 2008-12-10 10:44:33.000-0600

Repository: asterisk
Revision: 162670

U   branches/1.4/include/asterisk/stringfields.h

r162670 | mmichelson | 2008-12-10 10:44:33 -0600 (Wed, 10 Dec 2008) | 14 lines

Update to stringfield handling so that side-effects on
parameters are not evaluated multiple times.

An example where this caused a problem was in chan_sip.c, with
the line

 ast_string_field_set(p, fromdomain, ++fromdomain);

This patch was originally uploaded to issue ASTERISK-12965 by
jamessan. While the issue was closed for other reasons, this
patch is valid and fixes a separate problem, and is thus
being committed.