[Home]

Summary:ASTERISK-04362: [patch] Unquoted string comparisons fail in expression parser
Reporter:lancey (lancey)Labels:
Date Opened:2005-06-06 18:26:47Date Closed:2008-01-15 15:38:19.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) expr2.patch
( 1) expr2.patch2
( 2) expr2-doc.patch
Description:$[${variable} = something] always returns 1, no matter of variable's value and "something"

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

Asterisk CVS as of 2005-05-15 is fine. CVS 2005-06-01 and later expose the bug. It has been broken
Comments:By: Michael Jerris (mikej) 2005-06-06 19:39:41

Can you please step throught CVS day by day and narrow down what specific day broke this please.  You can do so with -D flag to CVS.  This will help us narrow down the issue.  Do > < work?
By: Tilghman Lesher (tilghman) 2005-06-07 11:18:10

What is your installed version of flex, i.e. output of 'flex -V'?
By: Tilghman Lesher (tilghman) 2005-06-07 11:32:34

I'm pretty sure I know what the issue is.  There's a new expression parser in CVS, which requires flex version 2.5.31.  Apparently there's a bug in the new parser with respect to string comparisons.  If you drop your installed flex version to before 2.5.31 (say, 2.5.4) and 'make clean install', I suspect this issue will go away, which will point directly to the new expression parser as the culprit.
By: Michael Jerris (mikej) 2005-06-07 11:57:27

murf-  Can you please take a look at this
By: lancey (lancey) 2005-06-07 18:38:58

Yup, Corydon76 seems to be right:
insomnia:~# flex -V
flex 2.5.31

I do not have time now to downgrade flex, nor step through the CVS day by day, but will do that tomorrow and confirm.
By: Steve Murphy (murf) 2005-06-07 22:18:45

First I've heard of this was now, because 2058 refs this bug.
Interesting. I will investigate and get back ASAP.
By: Steve Murphy (murf) 2005-06-07 23:30:02

OK, I've just done a round of testing on this, and I can't duplicate the problem.

First, I inserted a simple test into the context that covers
my phone extension:


exten => 775,1,GotoIf($[ ${EXTEN}=${EXTEN} ]?775|2:775|4)
exten => 775,2,BackGround,digits/1
exten => 775,3,Goto(775,6)
exten => 775,4,BackGround,digits/0
exten => 775,5,Goto(775,6)
exten => 775,6,GotoIf($[ ${EXTEN}=${LANGUAGE} ]?775|7:775|9)
exten => 775,7,BackGround,digits/1
exten => 775,8,Goto(775,11)
exten => 775,9,BackGround,digits/0
exten => 775,10,Goto(775,11)
exten => 775,11,BackGround,digits/2

Then, I 'extensions reload', and pick up my phone and dial 775.

It says "one"  "zero" "two"... what I'd expect. The first test
returns "1", the second "0". The two is make sure something gets
said.

Then, I updated my CVS to the current moment. Had a few glitches compiling...
what is "include/astconf.h"? Touching it into existence seemed to be needed to proceed with the make...

did a make install, and restarted asterisk, and picked up my phone, and dialed 775 again... all is still well.

If there is something grossly wrong in your extensions.conf, then I'd expect to see something about it in your /var/log/astersisk/messages file. Could you take a look and see if says anything?  

Another thing available is the testexpr2 program... If you say
"make testexpr2", it will build this small program. You can then
say "./testexpr2 good=bad"
and it should give you a zero, while "./testexpr2 good=good" should give
you a 1.

Let me know... it could be something specific to your architecture, which I don't see described above, maybe, also.
By: Steve Murphy (murf) 2005-06-07 23:54:50

One other thing!
you might remove ast_expr*.o ast_expr.a
and ast_expr2*.c, and remake. In the past,
I noticed that old code will not be remade
if not disturbed. I don't know how this
may or may not relate to your situation, but it's
worth a try.
By: Steve Murphy (murf) 2005-06-08 01:23:53

And even one MORE thing! ;^)

I just answered Boehm on the dev- mailing list, who had a similar problem,
where the conditional always came true because of a subtle affect in the syntax.

He had an expression that looked like this:

exten => _9.,1,GotoIf($[${CALLERIDNUM} : 3091|3144]?cscid:ncid)

His misconception was that 3091|3144 would act as a single pattern for the ":" operator.

But it does not. The "|" is an operator, and it is no longer hidden by the lack of spaces around it, as it was in the "old" parser. So, if you really want the "|" in the pattern, you'd say:

exten => _9.,1,GotoIf($[${CALLERIDNUM} : "3091|3144"]?cscid:ncid)

and that would work as expected.  Hopefully, this is not your problem. For full details, see the new README.variables file.

murf
By: Steve Murphy (murf) 2005-06-09 23:58:52

lancey-- haven't heard anything from you. If you haven't had a chance to debug this problem yet, here's my suggestion at the procedure to follow:

1. do a cvs update, go to the utils dir, and make check_expr. Then, run the generated check_expr program with the path to your extensions.conf. See it gives any warning about your expressions.

2. If so, fix 'em; if you fix them, report back and close this bug. If no warnings, or your fixes don't do anything, then... compile and run the testexpr program, and see if good=good and good=bad return 1 and 0 respectively.

3. If good=good and good=bad both return 1, report back. We have some work to do. Otherwise, check your /var/log/asterisk/messages file, and see if there are any syntax error messages. If there are, fix them as indicated. If you don't grok the problem, report here.

4. If there's no syntax errors, then try the advise about rebuilding the objects I gave previously. See if that helps.

Don't try to change your flex. Don't try to fetch previous CVS versions. The problem is clearly either your expressions (because of the differences in the new parser), or the problem is the expression parser. We need to fix it if so. Please help.
By: lancey (lancey) 2005-06-12 18:15:37

I was out of town for a while, sorry. I'm now starting to do what you supposed me to test and will get back here.
By: lancey (lancey) 2005-06-12 19:05:02

OK, here's the check_expr output:
insomnia:/usr/src/asterisk/utils# ./check_expr /etc/asterisk/extensions.conf
(skipped the expressions, just the summary)
Summary:
 Expressions detected: 39
 Expressions OK:  39
 Total # Warnings:   0
 Longest Expr:   26 chars
 Ave expr len:  17 chars

The testexpr2 results:
insomnia:/usr/src/asterisk# ./testexpr2 good=good
=====1======
insomnia:/usr/src/asterisk# ./testexpr2 good=bad
=====0======
insomnia:/usr/src/asterisk# ./testexpr2 "good = good"
=====1======
insomnia:/usr/src/asterisk# ./testexpr2 "good = bad"
=====0======

So far, so good, no errors in my expressions, no errors in expression logic according to testexpr2.

I'm now gonna play with my extensions and post here again later.
By: lancey (lancey) 2005-06-12 19:33:34

OK, i think i got it, the new expression parser seems to compare things as numbers, not as strings, unless explicitly quoted in "-s.

E.g.:
Extension:
==========
exten => 668,1,Set(TEST=00359889811777)
exten => 668,2,GotoIf($["${TEST}" = "00359889811777"]?3:5)
exten => 668,3,SayDigits(1)
exten => 668,4,Goto(6)
exten => 668,5,SayDigits(0)
exten => 668,6,GotoIf($["${TEST}" = "00359889811888"]?7:9)
exten => 668,7,SayDigits(1)
exten => 668,8,Hangup
exten => 668,9,SayDigits(0)
exten => 668,10,Hangup

Results:
========
   -- Executing GotoIf("SIP/supp1-a684", "1?3:5") in new stack
   -- Goto (voice1,668,3)
   -- Executing SayDigits("SIP/supp1-a684", "1") in new stack
   -- Playing 'digits/1' (language 'en')
   -- Executing Goto("SIP/supp1-a684", "6") in new stack
   -- Goto (voice1,668,6)
   -- Executing GotoIf("SIP/supp1-a684", "0?7:9") in new stack
   -- Goto (voice1,668,9)
   -- Executing SayDigits("SIP/supp1-a684", "0") in new stack
   -- Playing 'digits/0' (language 'en')
   -- Executing Hangup("SIP/supp1-a684", "") in new stack

Extension:
==========
exten => 668,1,Set(TEST=00359889811777)
exten => 668,2,GotoIf($[${TEST} = 00359889811777]?3:5)
exten => 668,3,SayDigits(1)
exten => 668,4,Goto(6)
exten => 668,5,SayDigits(0)
exten => 668,6,GotoIf($[${TEST} = 00359889811888]?7:9)
exten => 668,7,SayDigits(1)
exten => 668,8,Hangup
exten => 668,9,SayDigits(0)
exten => 668,10,Hangup


Results:
========
   -- Executing GotoIf("SIP/supp1-67f7", "1?3:5") in new stack
   -- Goto (voice1,668,3)
   -- Executing SayDigits("SIP/supp1-67f7", "1") in new stack
   -- Playing 'digits/1' (language 'en')
   -- Executing Goto("SIP/supp1-67f7", "6") in new stack
   -- Goto (voice1,668,6)
   -- Executing GotoIf("SIP/supp1-67f7", "1?7:9") in new stack
   -- Goto (voice1,668,7)
   -- Executing SayDigits("SIP/supp1-67f7", "1") in new stack
   -- Playing 'digits/1' (language 'en')
   -- Executing Hangup("SIP/supp1-67f7", "") in new stack

This WASN'T the way things were evaluated, so i think the new parser breaks backwards compatibility, and this wasn't noted anywhere.

If it will stay this way, i think check_expr must be updated so it checks for this and produces a warning.
By: Steve Murphy (murf) 2005-06-13 10:53:54

Lancey--

Excellent work. Your analysis helped me find the bug, which was that numeric strings should be left as strings, but declared to be "numeric_string". I missed this little twist, and converted (badly) such number strings to int. At best, it should have reported the overflow, but didn't.

I fixed it to follow the original interpretation, and threw a slightly better error message on overflows converting numeric_strings to long-longs.

Both the old and new versions of the parser convert numeric strings to ints before doing any of the binary ops (+,-,=,>, etc), and this will be OK until the numbers get over about 18-19 digits. At that point, the user should consider forcing string comparisons by wrapping the numbers in double quotes.

The old parser used strtoq to convert  the string to integer, but I modified that to strtoll in this patch, because my interpretation of the definition of the strtoq func could see it return 32-bit values on 32-bit machines. Those of you with BSD archs hopefully have a strtoll func! If not, we will have to rethink things.

I'll attach the patch, please download it, apply to your updated CVS version, and verify that dialing your test exensions yields "one", "zero" in the second (non-quoted) test case, and post here to let the maintainers know that the patch is OK to apply to CVS (or not).

Sorry for the inconvenience, many thanks for the feedback!
By: Tilghman Lesher (tilghman) 2005-06-13 15:45:02

murf:  there's no need in this patch to do an sprintf, then ast_log.  ast_log() already has the provision to take a format string, with variable number arguments.
By: Steve Murphy (murf) 2005-06-14 08:55:20

Good point. I've got it fixed here.  As soon as lancey gives thumbs up or down on the mods so far, I'll upload the tweaks.
By: Tilghman Lesher (tilghman) 2005-06-15 10:24:55

Since strtoll() is in ISO 9899 (C99) and POSIX 1003.1-2001, all platforms should have that function.  BSD platform is not an issue.
By: lancey (lancey) 2005-06-15 19:13:10

insomnia:/usr/src/asterisk# patch < expr2.patch
patching file ast_expr2.y
patching file ast_expr2.fl
Hunk #2 succeeded at 77 with fuzz 2.

(where is Hunk #1?)

Extension
=========
The test with unqouted strings.

Results
=======
   -- Executing GotoIf("SIP/supp1-2a6d", "1?3:5") in new stack
   -- Goto (voice1,668,3)
   -- Executing SayDigits("SIP/supp1-2a6d", "1") in new stack
   -- Playing 'digits/1' (language 'en')
   -- Executing Goto("SIP/supp1-2a6d", "6") in new stack
   -- Goto (voice1,668,6)
   -- Executing GotoIf("SIP/supp1-2a6d", "0?7:9") in new stack
   -- Goto (voice1,668,9)
   -- Executing SayDigits("SIP/supp1-2a6d", "0") in new stack
   -- Playing 'digits/0' (language 'en')
   -- Executing Hangup("SIP/supp1-2a6d", "") in new stack

Seems fine :) Thank you, Murf!

Thumbs up! ;>


By: Steve Murphy (murf) 2005-06-15 20:41:14

Thanks! I've uploaded expr2.patch2... apply at your convenience, Corydon76.

The reason for the hunk2 message is... well... I've got another bug open,
with a patch to add the ' cond ? expr_true :: expr_false ' capability.

Since both your fix and the conditional expression changes are in my source,
I hack out the conditional expr in the resulting patch. Hence the fuzz. It's OK,
and hunk 1 isn't mentioned because it worked OK.

We really aught to say something in the README.variables, about the length of the comparisons with numeric args, so people aren't too perplexed about the overflows. I'll add a patch for that here in a few seconds.
By: Mark Spencer (markster) 2005-06-16 20:32:07

Fixed in CVS head.  No application to stable.
By: Digium Subversion (svnbot) 2008-01-15 15:38:19.000-0600

Repository: asterisk
Revision: 5919

U   trunk/ast_expr2.fl
U   trunk/ast_expr2.y
U   trunk/doc/README.variables

------------------------------------------------------------------------
r5919 | markster | 2008-01-15 15:38:19 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix expression handling for string comparisions without quotes (bug ASTERISK-4362)

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

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