[Home]

Summary:ASTERISK-12131: [patch] Pattern matching treats 'x' differently than 'X'
Reporter:Jared Smith (jsmith)Labels:
Date Opened:2008-06-02 21:50:32Date Closed:2008-06-06 14:49:28
Priority:MinorRegression?No
Status:Closed/CompleteComponents:PBX/pbx_config
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) lower_case_patterns-1.4-v3.patch
( 1) lower_case_patterns-1.6.0-v1.patch
( 2) lower_case_patterns-trunk-v1.patch
Description:Steve Edwards reported [http://lists.digium.com/pipermail/asterisk-users/2008-June/212956.html] that the pattern _2xxx is evaluated before _2[1-4]00.  I went to reply that it shouldn't be that way, but in my testing, I found he was right.

I think this is a pretty serious bug in the pattern matching code.

As I understand it, Asterisk should be evaluating the patterns one digit at a time, from left to right.  Whenever more than one pattern could possibly match, the *most constrained* digit should match first.  In other words, given these two patterns, if I dialed 1234, the first one should match before the second, as it's a more constrained match in the third digit (it matches two possible values, while the other matches three).

_12[3-4]X
_12[3-5]X

The problem arises in that Asterisk seems to be treating the wildcard 'X' differently than 'x', and I can't figure out why.  Both of them should mean "any digit 0 through 9", but apparently they don't.  See below for more details:

****** STEPS TO REPRODUCE ******

Add the following to your extensions.conf file:

[test-1]
exten => _2XXX,1,NoOp(foo)
exten => _2[1-4]00,1,NoOp(bar)

[test-2]
exten => _2xxx,1,NoOp(foo)
exten => _2[1-4]00,1,NoOp(bar)

Reload your dialplan, and then look at the output of "dialplan show 2300@test-1" and "dialplan show 2300@test-2".  If 'x' and 'X' were treated the same, you'd see the same sorting order.  But you don't on either 1.2 or 1.4:

Asterisk 1.2.24



localhost*CLI> show dialplan 2300@test-1

[ Context 'test-1' created by 'pbx_config' ]

 '_2XXX' =>        1. NoOp(foo)                                  [pbx_config]

 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]



-= 2 extensions (2 priorities) in 1 context. =-

localhost*CLI> show dialplan 2300@test-2

[ Context 'test-2' created by 'pbx_config' ]

 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]

 '_2xxx' =>        1. NoOp(foo)                                  [pbx_config]

Asterisk 1.4.20


localhost*CLI> dialplan show 2300@test-1
[ Context 'test-1' created by 'pbx_config' ]
 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]
 '_2XXX' =>        1. NoOp(foo)                                  [pbx_config]

-= 2 extensions (2 priorities) in 1 context. =-
localhost*CLI> dialplan show 2300@test-2
[ Context 'test-2' created by 'pbx_config' ]
 '_2xxx' =>        1. NoOp(foo)                                  [pbx_config]
 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]


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

I personally could care less about the 1.2 results, as I know we fixed a lot of the pattern matching code between 1.2 and 1.4.  What really bugs me is the 1.4 code though -- why would 'x' be given a different matching priority than 'X', and a higher priority than '[1-4]'?  

I'm probably wrong, but I'll go out on a limb and guess that part of the code only deals with the capitalized pattern matching letters (N, X, Z) and treats the lower-case versions as a literal 'x' character, but that something else in the code then treats the lower-case 'x' as a match anyway.  I'll try to find my way through the code and see if I can make sense of it later, and if I can, I'll update the bug report with my findings.
Comments:By: Jared Smith (jsmith) 2008-06-02 21:53:21

Oh, by the way I need to say thanks to Steve Edwards for pointing out the issue, and to Andrew Oulton ([TK]D-Fender on IRC) and Jack Storm (JackEStorm on IRC) for helping me test this on a variety of different Asterisk boxes, just to make sure I'm not going insane.
By: Jared Smith (jsmith) 2008-06-02 22:31:50

More information... here's the output of trunk:

localhost*CLI> core show version
Asterisk SVN-trunk-r119839 built by jsmith @ localhost.localdomain on a i686 running Linux on 2008-06-03 03:24:59 UTC
localhost*CLI> dialplan show 2300@test-1
[ Context 'test-1' created by 'pbx_config' ]
 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]
 '_2XXX' =>        1. NoOp(foo)                                  [pbx_config]

-= 2 extensions (2 priorities) in 1 context. =-
localhost*CLI> dialplan show 2300@test-2
[ Context 'test-2' created by 'pbx_config' ]
 '_2xxx' =>        1. NoOp(foo)                                  [pbx_config]
 '_2[1-4]00' =>    1. NoOp(bar)                                  [pbx_config]

-= 2 extensions (2 priorities) in 1 context. =-

So even though the pattern-matching stuff has been changed/updated in trunk, this is still an issue.  I think I have a patch for 1.4 ready to go, but I'll hold off posting it until I figure out how to fix this in trunk as well.
By: Jared Smith (jsmith) 2008-06-02 22:46:49

OK, seems to me to be an easy one-line patch for the 1.4 branch, trunk, and the 1.6.0 branch.  Just because I'm crazy I'll post patches for all three versions, and save some poor developer the ten seconds it would take to port to the different branches.
By: Tilghman Lesher (tilghman) 2008-06-02 23:22:18

To summarize a discussion on IRC, the current behavior permits the lowercase version of the pattern match characters 'x', 'n', and 'z' to override the default matching priority.  I agree that this is not desireable in the way that the behavior is undocumented.  We have two choices:  a) we may document the behavior, as a feature to override the default matching order, or b) we may change how the lowercase operators work to emulate the uppercase operators.
By: Jared Smith (jsmith) 2008-06-02 23:28:19

Just to clarify... I think the two options should be:

1) treat a lower-case character (n, x, z) exactly the same as it's upper-case counterpart (N, X, Z) or

2) treat a lower-case character (n, x, z) as a *literal* character

I *do not* think we should treat the lower-case characters as "special ways of overriding the default matching order".  As for the other two options I mentioned, the element of least surprise (and comments in the source code, as well as being very familiar with the way people write pattern matchines) leads me to lean *very strongly* towards option number 1.
By: Leif Madsen (lmadsen) 2008-06-03 09:49:48

I agree with Jared's proposed option 1 -- that's how I always thought it worked anyways.

The reason I like number 1 better, is because we already have a way of making those letters literal with the following example:

exten => _[X]RAY-XXX,1,NoOp()
By: Tilghman Lesher (tilghman) 2008-06-03 09:59:42

blitzrage: I think one of us misunderstands how it presently works.  My understanding is that "_x" does not match a literal "x" character -- it matches the digits 0-9.  The difference is that it supercedes more specific matches, kind of an override for the present matching algorithm, making it match more like the 1.2 algorithm, for people who want that kind of override.
By: Jared Smith (jsmith) 2008-06-03 10:04:50

No Corydon76, I respectfully disagree.  I don't think the lower case 'x' was *ever* meant to be a high priority kind of override, as you suggest, and I've never seen anyone purposely use it for anything other than the functionality of the capital X.  We should really take this discussion to the -dev mailing list, however, to see what other people think, as I'm just one voice in the crowd.
By: Leif Madsen (lmadsen) 2008-06-03 11:44:38

Corydon76: Right, I guess I was speaking in terms of not being in favour of option 2.

As for the _2xxx having a higher priority of _2[1-4]XX, and thus higher than _2XXX I would say that would definitely be unintended behaviour, and that _2xxx should have the same weight as _2XXX.

I agree this probably needs to go to the -dev mailing list, where I'll express my opinions there :)
By: Digium Subversion (svnbot) 2008-06-06 14:48:32

Repository: asterisk
Revision: 121010

U   trunk/main/pbx.c

------------------------------------------------------------------------
r121010 | tilghman | 2008-06-06 14:48:30 -0500 (Fri, 06 Jun 2008) | 6 lines

Make extension match characters case-insensitive.
(closes issue ASTERISK-12131)
Reported by: jsmith
Patches:
      lower_case_patterns-trunk-v1.patch uploaded by jsmith (license 15)

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

http://svn.digium.com/view/asterisk?view=rev&revision=121010
By: Digium Subversion (svnbot) 2008-06-06 14:49:28

Repository: asterisk
Revision: 121011

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

------------------------------------------------------------------------
r121011 | tilghman | 2008-06-06 14:49:27 -0500 (Fri, 06 Jun 2008) | 14 lines

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

........
r121010 | tilghman | 2008-06-06 14:55:08 -0500 (Fri, 06 Jun 2008) | 6 lines

Make extension match characters case-insensitive.
(closes issue ASTERISK-12131)
Reported by: jsmith
Patches:
      lower_case_patterns-trunk-v1.patch uploaded by jsmith (license 15)

........

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

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