Summary:	ASTERISK-12531: AEL does not translate quoted strings correctly in 1.6
Reporter:	Dmitry Andrianov (dimas)	Labels:
Date Opened:	2008-08-06 09:48:58	Date Closed:	2008-09-03 12:01:28
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/PBX
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) ael-MSet.diff
Description:	I used to use constructs like    a = "test here"; in the Asterisk AEL scripts. These were working fine in 1.4 (translating to)    Set(a=$[ "test here"]) This does not work for 1.6 anymore although translates to exactly the same dialplan. After executing this line the value of a variable on 1.6 contains quotes. This results in: 1. The following construct:    b = "${a}"; does not work at all giving parse error [Aug  6 18:01:24] WARNING[29514]: ast_expr2.fl:437 ast_yyerror: ast_yyerror():  syntax error: syntax error, unexpected '<token>', expecting $end; Input: 2. when the variable being assigned is CALLERID(number) - this results in sending    From: ""test here"" <sip:xxxxxxxxxxx@1.2.3.4>;tag=as1539ede1 in the SIP INVITE header. Note the double quotes. As result, the Aastra 57i phone rejects the invite with 400 Bad Request.
Comments:	By: Leif Madsen (lmadsen) 2008-08-11 10:33:44 I'm not entirely sure if this is a bug in the AEL itself, or further down in the core as a bug in the dialplan. Can you try reproducing this outside of AEL straight in the dialplan? murf: please let me know if you don't think this is AEL and this bugs needs to be reassigned. By: Dmitry Andrianov (dimas) 2008-08-13 18:09:28 Hm... The problem is "solved" in SVN-trunk-r129270M. It looks like commit 120171 is the one which "fixes" the poblem. I put fixes in quotes because is it fixed or not actually depends on the compat setting. If [compat] section of asterisk.conf contains app_set=1.4 then the problem is "fixed" because old MSet is used instead of Set and AST_STANDARD_APP_ARGS macro there de-quotes strings. However if app_set=1.6 then Asterisk falls back to the "new" behavior I encountered and Set application does NOT work as for 1.4. And yes, the problem can be demonstrated without AEL too: exten => 098,1,Set(a="qqq") exten => 098,n,NoOp(-${a}-)    -- Executing [098@default:1] Set("SIP/1001-08f69f28", "a="qqq"") in new stack    -- Executing [098@default:2] NoOp("SIP/1001-08f69f28", "-"qqq"-") in new stack Bottom line: when not in 1.4 compat mode, the problem still present and it is not AEL related. By: Steve Murphy (murf) 2008-08-18 16:39:31 OK, I've been pondering what I **could** do in AEL to make your burden lighter, and it sounds like all I have to do is look for a set to a quoted string (in 1.6 and trunk only), and dequote the string before I put in the Set() call. This would only be done for <val>="<something>";  -- if it generates Set(<val>=<something>); instead of Set(<val>="<something>"); Would this kind of update be useful? I've been trying to think of issues that it might not solve... By: Dmitry Andrianov (dimas) 2008-08-18 17:12:31 Well, I think de-quoting in AEL may not be a good idea actually. I used to use quotation to prevent evaluation of expressions. That is:  a = 5;  b = 1;  c = "${a}-${b}"; If you convert last line to Set(a=$[${a}-${b}]) it will result in c=4 instead of c="5-1". Because of this I think, it is Set application that needs to be fixed... By: Steve Murphy (murf) 2008-08-18 17:32:01 Yes, I sat down and looked at it from a few more directions, and I hesitate to dequote the strings also. If there is ever a doubt, the best thing to do in AEL is just call the Set() application directly: Set(c=${a}-${b}); By: Dmitry Andrianov (dimas) 2008-08-18 17:49:45 Sure, but it kills most beauties of AEL... There is currently only one case in all my dialplans where I HAVE TO use Set instead of normal assignment. It is:  Set(row="${REALTIME(disausers,name,${ext},*,~)}"); (that is call to a function). In other cases I just use assignment. It would be best if the case above could be also handled with normal assignment. But it is definitely offtopic in context of this specific issue. By: Steve Murphy (murf) 2008-08-19 09:13:27 This is interesting. I just spent over an hour trying to repeat your results between 1.4 and 1.6.0, and could not see the difference. I had absolutely no [compat] section in my asterisk.conf file. So, I put it in, and set app_set = 1.6 and THEN I could see the results you mentioned. So, a closer study of the code shows ast_compat set to 7 initially; so the code acts like 1.4 by default. To get it to act like 1.6, you probably need to do a fresh install of 1.6.0, and do the "make samples", and let it write out the asterisk.conf for you, with app_set = 1.6 to get your behavior. If you had simply upgraded your 1.4 to 1.6, and kept the same config files, you would not have noticed the difference.... I see that under compat mode, the Multiple set is called, which uses the AST_STANDARD_APP_ARGS(args, data) to seperate prop=val pairs, and AST_NONSTANDARD_APP_ARGS(pair, args.pair[x], '=') to seperate the name from the value. NONSTANDARD is a macro that calls ast_app_separate_args (in main/app.c) to do the work, and it is the func that removes the quotes. Tilghman Lesher is gone for the rest of this week, and he is the authority as to why the new version of set was explicitly written to NOT dequote strings, and to NOT use the app-arg stuff in slicing and dicing the set argument string. These reasons would make great explanation in the doc strings in Set() and MSet(). I think what I will do is add verbage to the docstrings for Set() and MSet(). This will/should close this bug. When Tilghman gets back, he can further expound and clarify (redact) what I added. By: Steve Murphy (murf) 2008-08-19 09:20:41 Tilghman-- I'm having trouble with AEL in 1.6.x and trunk, because the underlying behavior has changed, and the automatic stripping of quoted strings is no longer taking place because the AST_NONSTANDARD_APP_ARGS() macro is not being used to split up the args. Hmmm. It occurs to me that one solution to this problem is to use MSet instead in the generated code of AEL, to preserve the previous behavior. This would lock in AEL, though, to forever use the old behavior. The app_set stuff in asterisk.conf is the alternate, but it's surprising upgrading users who fresh install asterisk 1.6.x and get the 1.6 behavior. By: Steve Murphy (murf) 2008-08-19 09:26:40 I think I'll leave this bug open until Tilghman gets back from vacation and we can discuss this. In the meantime, I can have AEL generate a warning/notice that quoted strings when app_set is 1.6, will not be stripped as they were in 1.4. After discussion, we can make a final decision of further action pertaining to this bug. By: Dmitry Andrianov (dimas) 2008-08-19 09:28:12 murf, that is exactly what I was tying to say in my note 0091394. But as I see it was anything but clear :) By: Steve Murphy (murf) 2008-08-19 12:13:58 OK, I did update AEL to check for this type of situation and issue a warning at runtime, as AEL is loading and parsing the dialplan. The standalones won't do because they don't load asterisk.conf; however, if there is sufficient demand, I can update the AEL standalone tools (aelparse) to load asterisk.conf and determine if this is a possible problem situation and issue the warning also. See revs 138815 & 138845 in trunk; 138846 and 138847 in 1.6.0, and 138853 and 138855 in 1.6.1 By: Dmitry Andrianov (dimas) 2008-08-21 04:43:34 murf, I just realized that your idea to convert AEL assignment into MSet is may be wrong. It will break scripts where comas/pipes are in the RHS of assignment. So I believe, the new Set behavior still needs to be fixed... Lets see what Tilghman thinks... By: Tilghman Lesher (tilghman) 2008-08-24 10:32:04 I think the correct response would be to remove quotes when translating into loaded dialplan, as, while they may be necessary in AEL, they are unnecessary after being translated to extensions.conf.  If literal quotes are needed, they should be escaped, i.e. CALLERID(all) = "\"foo bar\" <baz>"; should translate to Set(CALLERID(all)="foo bar" <baz>). By: Steve Murphy (murf) 2008-08-26 16:30:42 Which brings me back to using the MSet() app to assign a value to a variable. It will do all the things that used to be done with the previous version. If I were to write code to strip the surrounding quotes from the input, I would just copy the code in the ast_app_separate_args code, which is what gets called by MSet(). It would simply replicate what asterisk and AEL used to do before app_set=1.6. It would only do this, of course, if app_set=1.6. dimas-- you say it will break scripts where commas/pipes are in the RHS of an assignment, but I'm not seeing the problem-- if your scripts worked under 1.4, with MSet, they should continue to work when app_set=1.6. By: Dmitry Andrianov (dimas) 2008-08-26 16:41:43 murf, my scripts were working under 1.4 with notmal Set (because AEL translates assignment to a Set). Since old Set was de-quoting, strings it was workign fine. If you change assignment to MSet, it will not only do de-quoting but will perform multiple sets which is not the same... By: Steve Murphy (murf) 2008-08-26 17:43:57 Well, actually, the old 1.4 Set() did multiples. If you look at the pbx_builtin_setvar function, you'll see that it calls the ast_app_separate_args with a '|' separator; then loops on the arg list and sets the vars. A little used feature, to be sure, but it's the same code as MSet(). You are welcome to compare. By: Dmitry Andrianov (dimas) 2008-08-28 06:39:59 murf, Are you sure? the 1.4 AEL assignment were issuing warnings on the console... Also, as I understand, AEL wraps any assignment into $[...] so how these pipes woll work there? By: Steve Murphy (murf) 2008-08-28 10:18:21 I'm very sure. I just attached a patch to this bug. It removes the warning, and uses MSet() if you don't have the 1.6 setting for compat. See if it behaves exactly like your 1.4 based stuff. By: Steve Murphy (murf) 2008-08-29 15:09:09 dimas-- please test out the attached patch on 1.6.x or trunk, and tell me of any problems... By: Steve Murphy (murf) 2008-09-02 22:31:51 Hmmm. I'm going to assume that you have no problems with the proposed code, then, and merge it into the repository for trunk and 1.6.1... Tomorrow sometime, unless you object... OK? By: Dmitry Andrianov (dimas) 2008-09-03 01:49:45 hey-hey, you only gave me three days :) anyway, I just tested your patch and it seems it works fine in my test environment. Unfortunately, I can not test my "real" dialplan there. Will re-test everything whn I will be preparing for upgrade to 1.6 next time. This time I had to rollback live systems to 1.4 By: Steve Murphy (murf) 2008-09-03 12:01:26 This bug is closed via svn rev 140824 in trunk, and 140886 in 1.4. (I guess I didn't format the commit msg correctly) URL: http://svn.digium.com/view/asterisk?view=rev&rev=140886 Log: Merged revisions 140824 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r140824 | murf | 2008-09-03 08:01:27 -0600 (Wed, 03 Sep 2008) | 21 lines In these changes, I have added some explanation of changes to the Set and MSet apps, so people aren't so shocked and surprised when they upgrade from 1.4 to 1.6. Also, for the sake of those upgrading from 1.4 to 1.6 with AEL, I provide automatic support for the "old" way of using Set(), that still does the exact same old thing with quotes and backslashes and so on as 1.4 did, by having AEL compile in the use of MSet() instead of Set(), everywhere it inserts this code. But, if the app_set var is set to 1.6 or higher, it uses the "new", non-evaluative Set(). This only usually happens if the user manually inserts this into the asterisk.conf file, or runs the "make samples" command. (closes issue ASTERISK-12531) Reported by: dimas Patches:      ael-MSet.diff uploaded by murf (license 17) Tested by: dimas, murf ........ Modified:    branches/1.6.1/   (props changed)    branches/1.6.1/main/pbx.c    branches/1.6.1/res/ael/ael.tab.c    branches/1.6.1/res/ael/ael.tab.h    branches/1.6.1/res/ael/ael.y    branches/1.6.1/res/ael/pval.c