[Home]

Summary:ASTERISK-16909: [patch] Realtime field 'fullcontact' populated with invalid data
Reporter:Nick Barnes (bcnit)Labels:
Date Opened:2010-11-04 09:55:30Date Closed:2011-02-24 08:05:48.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/DatabaseSupport
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20101110__issue18251.diff.txt
( 1) 20101110a__issue18251.diff.txt
( 2) sipbug.log
Description:
Using Asterisk realtime, the field 'fullcontact' is populated with invalid data. The result is that warnings occur when a handset re-registers after Asterisk has been restarted.

In my case, after a handset registers, the field contains something like:

'spts211..52:04ln=2lbnso30731'

The warnings are:

[Nov  4 14:50:29] WARNING[5157]: chan_sip.c:12302 __set_address_from_contact: Invalid contact uri spts211..52:04ln=2lbnso30731 (missing sip: or sips:), attempting to use anyway
[Nov  4 14:50:29] WARNING[5157]: chan_sip.c:12321 __set_address_from_contact: Invalid host name in Contact: (can't resolve in DNS) : 'spts211..52'


****** ADDITIONAL INFORMATION ******


The back end database is MySQL accessed over ODBC.

The 'fullcontact' field is defined as a varchar(80).

I understand that the field should contain something like:

'sip:200@10.155.66.6:2051;line=wkya7tfd'

which works fine on our older systems.

Comments:By: Nick Barnes (bcnit) 2010-11-04 10:00:56

Should say that the result of this bug is that handsets are unreachable after restarting asterisk until they re-register. This is the reason it has been raised as 'major'.
By: Paul Belanger (pabelanger) 2010-11-04 12:09:39

I believe Leif had the same issue, however not if he resolved it.  In the meantime, please upload a debug log (see below), with SIP debugs enabled.
---
We require a complete debug log to help triage the issue.

This document will provide instructions on how to collect debugging logs from an Asterisk machine for the purpose of helping bug marshals troubleshoot an issue:

https://wiki.asterisk.org/wiki/display/AST/Collecting+Debug+Information
By: Nick Barnes (bcnit) 2010-11-04 12:33:01

Sip/debug log attached.

Looks like res_config_odbc is being passed the correct value:

[Nov  4 17:16:05] DEBUG[6838] res_config_odbc.c: Parameter 7 ('fullcontact') = 'sip:test201@10.6.15.25:2054;line=r794bef0'

But only every other character is being stored in the database:

spts211..52:04ln=74e0


By: Nick Barnes (bcnit) 2010-11-05 04:46:20

Right, I think I've found the problem.

in res/res_config_odbc.c there is a for-loop which cycles through every character in the string:

for (; *vptr && eptr < encodebuf + sizeof(encodebuf); vptr++)

The problem appears to be that there are two occurrances of vptr++ elsewhere:

*eptr++ = *vptr++;
and
vptr++;

If these are changed to:

*eptr++ = *vptr;
and
// LINE DELETED!

Then the characters aren't skipped.

However, the semi-colon is substituted with "^3B" so the line to be stored is:

sip:test201@10.6.15.25:2054;line=z3mscbjz

and what's actually stored is:

sip:test201@10.6.15.25:2054^3Bline=z3mscbjz

It would appear that although "^3B" is inserted in the string, at some point this is meant to be translated back to a ";" before it is stored in the database. This doesn't happen.

I have temporarily resolved this problem by making the two changes to the '++' as outlined above and then commenting out the chunk of code which replaces the ';' in the first place. It all now appears to be working well, but I appreciate that this is a hack and may leave me open to an injection attack.....
By: Paul Belanger (pabelanger) 2010-11-05 07:34:03

You'll have to submit your patch to the tacker, not attach them as a note.

---
Please use `svn diff` on all your patches. Patches which include alternate formatting are almost certainly going to be thrown out or ignored; there are too few hours in the day to wade through difficult-to-follow C code fixes without the help of diff -u.
By: Nick Barnes (bcnit) 2010-11-05 07:37:22

Hi,

I'm not a programmer - what I've done is a hack, not a patch!

I have identified where the problem is and have essentially bypassed it.

I have no idea what the implications of what I have done are.

I have no idea whether what I have done is correct or not.

My notes listed above were added simply to give as much information as possible to help somebody else identify and solve the problem.

Sorry, but I can't do any more than I have done and I certainly don't want my hack making its way into the distribution!
By: Tilghman Lesher (tilghman) 2010-11-10 11:43:47.000-0600

The code to replace the semicolon is intentional, to prevent the Contact field from being split into two values when it is read back out of the database.  Removing it will guarantee that the Contact field will be invalid.
By: Mark Willis (cartama) 2010-11-10 14:24:44.000-0600

I've attached a simpler fix that works for me with MS-SQL as the backend.
By: Trevor Peirce (trev) 2010-11-12 23:34:53.000-0600

20101110__issue18251.diff.txt works for me (MySQL although I doubt that's relevant).
By: Jonathan Thurman (jthurman) 2010-11-14 13:37:39.000-0600

20101110__issue18251.diff.txt resolved the issue for me on 1.8.0.
By: Michael L. Young (elguero) 2010-11-16 08:19:19.000-0600

Add another confirmation that this patch (20101110__issue18251.diff.txt) works... I tested it with trunk.
By: Paul Belanger (pabelanger) 2010-11-16 08:24:04.000-0600

Promoted to 'Ready for Review'
By: Digium Subversion (svnbot) 2010-12-16 03:03:41.000-0600

Repository: asterisk
Revision: 298480

U   branches/1.4/res/res_config_odbc.c

------------------------------------------------------------------------
r298480 | tilghman | 2010-12-16 03:03:41 -0600 (Thu, 16 Dec 2010) | 14 lines

Only increment the pointer once per loop, otherwise we corrupt the value.

(closes issue ASTERISK-16909)
Reported by: bcnit
Patches:
      20101110__issue18251.diff.txt uploaded by tilghman (license 14)
Tested by: trev, jthurman, elguero

(closes issue ASTERISK-16932)
Reported by: zerohalo
Patches:
      20101109__issue18279.diff.txt uploaded by tilghman (license 14)
Tested by: zerohalo

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

http://svn.digium.com/view/asterisk?view=rev&revision=298480
By: Digium Subversion (svnbot) 2010-12-16 03:04:39.000-0600

Repository: asterisk
Revision: 298481

_U  branches/1.6.2/
U   branches/1.6.2/res/res_config_odbc.c

------------------------------------------------------------------------
r298481 | tilghman | 2010-12-16 03:04:39 -0600 (Thu, 16 Dec 2010) | 21 lines

Merged revisions 298480 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r298480 | tilghman | 2010-12-16 03:03:40 -0600 (Thu, 16 Dec 2010) | 14 lines
 
 Only increment the pointer once per loop, otherwise we corrupt the value.
 
 (closes issue ASTERISK-16909)
  Reported by: bcnit
  Patches:
        20101110__issue18251.diff.txt uploaded by tilghman (license 14)
  Tested by: trev, jthurman, elguero
 
 (closes issue ASTERISK-16932)
  Reported by: zerohalo
  Patches:
        20101109__issue18279.diff.txt uploaded by tilghman (license 14)
  Tested by: zerohalo
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=298481
By: Digium Subversion (svnbot) 2010-12-16 03:05:29.000-0600

Repository: asterisk
Revision: 298482

_U  branches/1.8/
U   branches/1.8/res/res_config_odbc.c

------------------------------------------------------------------------
r298482 | tilghman | 2010-12-16 03:05:29 -0600 (Thu, 16 Dec 2010) | 28 lines

Merged revisions 298481 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

................
 r298481 | tilghman | 2010-12-16 03:04:38 -0600 (Thu, 16 Dec 2010) | 21 lines
 
 Merged revisions 298480 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r298480 | tilghman | 2010-12-16 03:03:40 -0600 (Thu, 16 Dec 2010) | 14 lines
   
   Only increment the pointer once per loop, otherwise we corrupt the value.
   
   (closes issue ASTERISK-16909)
    Reported by: bcnit
    Patches:
          20101110__issue18251.diff.txt uploaded by tilghman (license 14)
    Tested by: trev, jthurman, elguero
   
   (closes issue ASTERISK-16932)
    Reported by: zerohalo
    Patches:
          20101109__issue18279.diff.txt uploaded by tilghman (license 14)
    Tested by: zerohalo
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=298482
By: Digium Subversion (svnbot) 2010-12-16 03:06:21.000-0600

Repository: asterisk
Revision: 298483

_U  trunk/
U   trunk/res/res_config_odbc.c

------------------------------------------------------------------------
r298483 | tilghman | 2010-12-16 03:06:21 -0600 (Thu, 16 Dec 2010) | 35 lines

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

................
 r298482 | tilghman | 2010-12-16 03:05:28 -0600 (Thu, 16 Dec 2010) | 28 lines
 
 Merged revisions 298481 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ................
   r298481 | tilghman | 2010-12-16 03:04:38 -0600 (Thu, 16 Dec 2010) | 21 lines
   
   Merged revisions 298480 via svnmerge from
   https://origsvn.digium.com/svn/asterisk/branches/1.4
   
   ........
     r298480 | tilghman | 2010-12-16 03:03:40 -0600 (Thu, 16 Dec 2010) | 14 lines
     
     Only increment the pointer once per loop, otherwise we corrupt the value.
     
     (closes issue ASTERISK-16909)
      Reported by: bcnit
      Patches:
            20101110__issue18251.diff.txt uploaded by tilghman (license 14)
      Tested by: trev, jthurman, elguero
     
     (closes issue ASTERISK-16932)
      Reported by: zerohalo
      Patches:
            20101109__issue18279.diff.txt uploaded by tilghman (license 14)
      Tested by: zerohalo
   ........
 ................
................

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

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