[Home]

Summary:ASTERISK-13477: [patch] Putting a comma in an extension dialpattern causes eventual seg fault
Reporter:nick_lewis (nick_lewis)Labels:
Date Opened:2009-01-29 07:25:54.000-0600Date Closed:2009-02-03 18:46:26.000-0600
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:Core/PBX
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090129__bug14362.diff.txt
( 1) pbx.c-dialcomma.patch
( 2) pbx.c-dialcomma2.patch
( 3) pbx.c-dialcomma3.patch
( 4) pbx.c-dialcomma4.patch
( 5) pbx.c-dialpattern.patch
Description:If there is erroneously a comma in an extension dialpattern such as

_9[1-3,5-9].

this leads some time later to a segmentation fault by various *s2=??? assignments in add_exten_to_pattern_tree() of pbx.c

There looks to be a string-unbounded while loop causing it
Comments:By: Leif Madsen (lmadsen) 2009-01-29 07:35:33.000-0600

I would agree that the segfault needs to be fixed, but your syntax is incorrect. You should not be using the comma there.

The correct syntax is without the comma:

_9[1-35-9].
By: nick_lewis (nick_lewis) 2009-01-29 07:35:38.000-0600

Please find attached a patch that may mitigate the problem
By: Tilghman Lesher (tilghman) 2009-01-29 13:16:14.000-0600

There are two things I'm doing.  The first is to incorporate your patch, because it clearly is needed.  For example, if someone typoed their config, then it should error out correctly.

Secondarily, I'm creating a patch which allows you to legitimately code a comma into an extension, as long as it exists within square brackets, as you have done here (unnecessarily, in your case), in case somebody wants to actually match a comma character in an extension.

Patch has been uploaded and initially tested.  Let's see if you can find a case to crash it.
By: nick_lewis (nick_lewis) 2009-01-30 03:39:16.000-0600

Another way of supporting comma characters that may be more consistent with the way square bracket characters are supported is to escape them with a preceding backslash for example

exten => _\[funny\,strange&weird\][\,\-;]\\etc\\,1,noop()

would match on

[funny,strange&weird],\etc\
[funny,strange&weird]-\etc\
[funny,strange&weird];\etc\
By: nick_lewis (nick_lewis) 2009-01-30 06:06:40.000-0600

Please find attached a patch that should escape a comma from anywhere in an extension dialpattern.
By: nick_lewis (nick_lewis) 2009-01-30 06:12:06.000-0600

Sorry I accidentally uploaded an early attempt of the comma escape patch. Please find real one attached
By: nick_lewis (nick_lewis) 2009-01-30 06:35:28.000-0600

My dialcomma patch makes changes only to the parsing of Goto(). It needs to do the same to pbx_load_config too

I notice that there is now a pbx_strsep patch. This only makes changes to pbx_load_config. I think it needs to make changes to parsing of Goto() as well.
By: Leif Madsen (lmadsen) 2009-01-30 08:02:48.000-0600

exten => _\[funny\,strange&weird\][\,\-;]\\etc\\,1,noop()

I would strong advocate *against* that format.

It is ugly, and makes writing further lines of dialplan extremely hard (although I suppose now we have the 'same' priority... so it might not be quite so bad).

But still, my first reaction to seeing that syntax was negative :)
By: nick_lewis (nick_lewis) 2009-01-30 08:32:03.000-0600

The only reason I suggested it is that it is already done this way for other characters such as
[ ] -

I agree that the example dialpattern is a bit ugly but all of it with the exception of backslash comma is the current implementation. i.e.

exten => _\[strange&weird\][\-;]\\etc\\,1,noop()

Perhaps it also needs to be possible to do this in a more attractive way. Is this what the paren and quote bits of pbx_strsep() do?

Anyway if using a special pbx_strsep() then I think it needs to be used in pbx_parseable_goto() too
By: Tilghman Lesher (tilghman) 2009-02-01 23:15:34.000-0600

I'm not sure why it would need to be used within pbx_parseable_goto.  Please make the case.  The point of allowing a comma in there isn't for use in the dialplan -- it's to match incoming extensions, where a SIP client, for example, might embed a comma in the middle of a phone number.  The only reason for using pbx_strsep is to deal with the square brackets -- something which CANNOT occur within an extension or else it will not match the specified pattern.  Otherwise, you could just use the standard application argument parser.  You could make the argument for the standard application parser, but pbx_strsep doesn't make any sense there.


By: nick_lewis (nick_lewis) 2009-02-02 02:59:45.000-0600

Re "I'm not sure why it would need to be used within pbx_parseable_goto"
I have not studied the code in detail but in principle if there is a line in extensions.conf such as

[mycontext]
exten => _123[,]456,1,noop()

and you want to have a goto that jumps to it then it would be something like

exten => s,n,Goto(mycontext,_123[,]456,1)

Re "square brackets -- something which CANNOT occur within an extension"
The function add_exten_to_pattern_tree() does seem to accept the square bracket in an extension as long as it is escaped with a backslash.

I fear that we are talking at cross purposes. Can you please give an example of what you mean
By: Tilghman Lesher (tilghman) 2009-02-02 07:58:59.000-0600

Nick_Lewis:

Please define
exten => _123,1,noop()
exten => 456,1,Goto(_123,1)

Now tell me why this won't work.  If you think it will work, please try it.
By: nick_lewis (nick_lewis) 2009-02-02 10:43:58.000-0600

Yes I have tried it and it seems to work fine but it does not seem to address the issue. If you add support for [,] to the extension field after exten=> then the same support will need to be added to the extension field in the Goto()
By: nick_lewis (nick_lewis) 2009-02-02 10:55:27.000-0600

Test Config & Results

[from-internal]
exten => _123,1,noop(ending here)
exten => 456,1,noop(starting here)
exten => 456,2,Goto(_123,1)

CLI>
-- Executing [456@from-internal:1] NoOp("SIP/1001-b760b9b8", "starting here") in new stack
-- Executing [456@from-internal:2] Goto("SIP/1001-b760b9b8", "_123,1") in new stack
-- Goto (from-internal,_123,1)
-- Executing [_123@from-internal:1] NoOp("SIP/1001-b760b9b8", "ending here") in new stack
By: Tilghman Lesher (tilghman) 2009-02-02 12:55:59.000-0600

Okay, more specifically, WHY do you want to put a comma in the middle of an extension?
By: nick_lewis (nick_lewis) 2009-02-03 03:28:09.000-0600

Corydon76 - Was it not you yourself that first raised the ability to legitimately have commas
Note 99075: "Secondarily, I'm creating a patch which allows you to legitimately code a comma into an extension, as long as it exists within square brackets, as you have done here (unnecessarily, in your case), in case somebody wants to actually match a comma character in an extension."
By: Tilghman Lesher (tilghman) 2009-02-03 12:27:02.000-0600

Yes, but the point of that, as I've already said, is to deal with incoming extensions, such as SIP dialstrings.  I have no intention of supporting a Goto with a comma in the middle of an extension.
By: Digium Subversion (svnbot) 2009-02-03 18:44:24.000-0600

Repository: asterisk
Revision: 173311

U   trunk/main/pbx.c
U   trunk/pbx/pbx_config.c

------------------------------------------------------------------------
r173311 | tilghman | 2009-02-03 18:44:24 -0600 (Tue, 03 Feb 2009) | 10 lines

Ensure that commas placed in the middle of extension character classes do not
interfere with correct parsing of the extension.  Also, if an unterminated
character class DOES make its way into the pbx core (through some other
method), ensure that it does not crash Asterisk.
(closes issue ASTERISK-13477)
Reported by: Nick_Lewis
Patches:
      20090129__bug14362.diff.txt uploaded by Corydon76 (license 14)
Tested by: Corydon76

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

http://svn.digium.com/view/asterisk?view=rev&revision=173311
By: Digium Subversion (svnbot) 2009-02-03 18:45:33.000-0600

Repository: asterisk
Revision: 173312

_U  branches/1.6.0/
U   branches/1.6.0/main/pbx.c

------------------------------------------------------------------------
r173312 | tilghman | 2009-02-03 18:45:33 -0600 (Tue, 03 Feb 2009) | 17 lines

Merged revisions 173311 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r173311 | tilghman | 2009-02-03 18:43:52 -0600 (Tue, 03 Feb 2009) | 10 lines
 
 Ensure that commas placed in the middle of extension character classes do not
 interfere with correct parsing of the extension.  Also, if an unterminated
 character class DOES make its way into the pbx core (through some other
 method), ensure that it does not crash Asterisk.
 (closes issue ASTERISK-13477)
  Reported by: Nick_Lewis
  Patches:
        20090129__bug14362.diff.txt uploaded by Corydon76 (license 14)
  Tested by: Corydon76
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=173312
By: Digium Subversion (svnbot) 2009-02-03 18:46:26.000-0600

Repository: asterisk
Revision: 173313

_U  branches/1.6.1/
U   branches/1.6.1/main/pbx.c

------------------------------------------------------------------------
r173313 | tilghman | 2009-02-03 18:46:25 -0600 (Tue, 03 Feb 2009) | 17 lines

Merged revisions 173311 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r173311 | tilghman | 2009-02-03 18:43:52 -0600 (Tue, 03 Feb 2009) | 10 lines
 
 Ensure that commas placed in the middle of extension character classes do not
 interfere with correct parsing of the extension.  Also, if an unterminated
 character class DOES make its way into the pbx core (through some other
 method), ensure that it does not crash Asterisk.
 (closes issue ASTERISK-13477)
  Reported by: Nick_Lewis
  Patches:
        20090129__bug14362.diff.txt uploaded by Corydon76 (license 14)
  Tested by: Corydon76
........

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

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