Summary: | ASTERISK-01284: [patch] ast_expr can't handle quoted strings, bad error messages, multiple spaces... | ||
Reporter: | Steve Murphy (murf) | Labels: | |
Date Opened: | 2004-03-24 15:31:46.000-0600 | Date Closed: | 2008-01-15 14:53:09.000-0600 |
Priority: | Major | Regression? | No |
Status: | Closed/Complete | Components: | Core/General |
Versions: | Frequency of Occurrence | ||
Related Issues: | |||
Environment: | Attachments: | ( 0) ast_expr.y.diffu ( 1) ast_expr.y.diffu2 | |
Description: | ast_expr can't handle strings with spaces, which pretty much rules out using ${CALLERIDNAME} in it, or any other variable that MIGHT contain a value with a space. Parse errors give no clue at all as to what was wrong. An extra space in the wrong place and you get a parse error. Not enough spaces, and it will either just plain not work, or yield a parse error. I've made a few small changes to look for quotes as well as spaces, and to end the token with the closing double quote. Quotes are removed from the token. Now, you can rest assured that this kind of construct will parse successfully: exten => s,6,GotoIf($[ ${CALLERIDNUM} : "3077545675" & "${CALLERIDNAME}" : "Privacy Manager" ]?telemarket|s|1:s|7) In the above, note that the stuff in $[] includes leading multiple spaces, trailing multiple spaces, quotes, and it works as expected, without parse errors. To improve parse error messages, I've added two static variables, ast_expr_argument_position, and ast_expr_argument, which keep track of what is being parsed, and where in the string we are parsing. If a parse error occurs, a message is generated, the input string is output, and a series of spaces, with an up-arrow character indicating the column in which the parser was scanning, is output in the next line. Thus, the end user sees any substitutions, and most likely which token might have created the problem. I've tested the positive branch, the negative branch, actual parse errors, etc. Looks good. | ||
Comments: | By: Steve Murphy (murf) 2004-03-26 18:24:49.000-0600 OK, I've just added a new version of the patch. The fact that I was using globals in a threaded environment to go to the yyerror routine really bothered me. So I did the homework, and played around with bison, and managed to use the right combo of things to pass the lex data to the yyerror routine to generate a thread-safe, useful error message. Now, the yyerror generates two lines following the input it got. The first line reflects data it gets from the location tracking data that bison can generate. Basically, it seems to indicate how much the parser has already parsed that was OK. The second line has an arrow at the position where the lookahead token ended. The error would most likely be between the last arrow on the first line, and the arrow on the second line. Usually this will make it pretty obvious, where the trouble lies. By: twisted (twisted) 2004-04-29 09:31:08 I believe some testing is in order here.. I can see where this woudl be useful.. By: Mark Spencer (markster) 2004-05-01 21:46:26 Testing would be nice, since i'm totally lost about lex... I made a "B" in compilers :( By: Steve Murphy (murf) 2004-05-01 23:50:15 Suggestion: IF there is any problems, the area to study is backwards compatibility. The features added will have these kinds of effects: 1. multiple spaces OK. This won't affect anyone with already written expressions, because they either chopped the extra spaces, or their expr never parsed. The guys who never fixed the broken exprs may suddenly find that they now work. This could be highly inconvenient, if they were depending on it NOT working...! 2. Things in double quotes now eval to a single token. This is only a factor if the string in double quotes contained spaces, and that was GOOD. This could be the case if perhaps the text evaluates to legal expr syntax. This would indeed be a fairly advanced usage of the exprs in Asterisk. I'd personally be interested in what they were doing! At any rate, it seems such could be fixed by tweeking the input strings. 3. Better syntax error messages. Well, no-one should suffer here, only if it crashes... I've run the code thru various tests here, and it seems to work as advertised. I'm going to delete the first version of this patch, so it isn't tempting anyone to use it. By: Mark Spencer (markster) 2004-05-01 23:54:19 Sure, what the heck, i'll put it in. I assume you have a disclaimer on file? By: Steve Murphy (murf) 2004-05-01 23:57:37 Pretty sure I sent one in. If not, I'll be happy to file another. Oh, by the way, I guess I can't delete an attached file. Thought under the "advanced" stuff, I'd be able to... sorry. By: Mark Spencer (markster) 2004-05-02 00:02:36 I'll take your word for it on the disclaimer :) Thank you very much for this generous contribution and if all heck breaks loose, we'll open this back up. By: Digium Subversion (svnbot) 2008-01-15 14:53:09.000-0600 Repository: asterisk Revision: 2855 U trunk/ast_expr.y ------------------------------------------------------------------------ r2855 | markster | 2008-01-15 14:53:08 -0600 (Tue, 15 Jan 2008) | 2 lines Merge murf's generous expression contributions (bug ASTERISK-1284) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=2855 |