[Home]

Summary:ASTERISK-13358: [patch] Parsing and escaping of characters is broken in some cases (e.g. app_System)
Reporter:John Covert (jcovert)Labels:
Date Opened:2009-01-13 11:12:03.000-0600Date Closed:2009-02-20 15:35:19.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/Configuration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090113__bug14231__2.diff.txt
( 1) app_system.c_app.h_strings.h_main_app.c_main_pbx.c_patch-1.6.0.3
( 2) app_system.c.patch-1.6.0.3
( 3) corrected_20090113__bug14231__2.diff.txt
Description:It is no longer possible to pass a semicolon (an important shell construct) to a System command.

I started writing this up specific to the System command, and actually developed a patch for it in the system command, but the problem is more widespread, and needs to be addressed globally, not as a point solution in each command.

In 1.4, it was possible to construct a System command such as the following:

exten => h,n,System(printf "Channel: local/${ThisExt}@localstations\\nContext: dialstation${ThisExt}\\nExtension: 811\\nCallerID: ${ThisExt}" >/var/spool/asterisk/ringback$$\;(sleep 1\;mv /var/spool/asterisk/ringback$$ /var/spool/asterisk/outgoing/ringback$$)&)

When executed, the escaped characters were removed by the parser before reaching the System command:

-- Executing [h@ringback:2] System("SIP/x28-0188be00", "printf "Channel: local/28@localstations\nContext: dialstation28\nExtension: 811\nCallerID: 28" >/var/spool/asterisk/ringback$$;(sleep 1;mv /var/spool/asterisk/ringback$$ /var/spool/asterisk/outgoing/ringback$$)&") in new stack

In 1.6.0.3, the escaped characters, except for an escaped '\', are passed directly through.  That most characters do not need to be escaped any more could be seen as "good" except that the semicolon does still need to be escaped, or the rest of the command is lost due to being considered a comment.

While my patch to System solves the problem there, the problem exists in most
other places you might need to insert a semicolon, such as in the Set Command.
Consider the following in 1.6:

exten => 954,1,Noop(Generic test.  ${CALLERID})
exten => 954,n,Set(var=a,b, ,d\;e\;f,g)
exten => 954,n,noop(${var})
exten => 954,n,noop(${CUT(var,\,,2)})
exten => 954,n,noop(${CUT(var,\;,2)})
exten => 954,n,Noop(onward....)
exten => 954,n,Hangup

Note in the output below that the variable ends up with the escapes before the semicolons included. Fortunately, the CUT command handles the escaping correctly.


   -- Goto (dialstation,954,1)
   -- Executing [954@dialstation:1] NoOp("SIP/x28-01894200", "Generic test.  ") in new stack
   -- Executing [954@dialstation:2] Set("SIP/x28-01894200", "var=a,b, ,d\;e\;f,g") in new stack
   -- Executing [954@dialstation:3] NoOp("SIP/x28-01894200", "a,b, ,d\;e\;f,g") in new stack
   -- Executing [954@dialstation:4] NoOp("SIP/x28-01894200", "b") in new stack
   -- Executing [954@dialstation:5] NoOp("SIP/x28-01894200", "e\") in new stack
   -- Executing [954@dialstation:6] NoOp("SIP/x28-01894200", "onward....") in new stack
   -- Executing [954@dialstation:7] Hangup("SIP/x28-01894200", "") in new stack


In 1.4, the Set command needed to be constructed differently:
Set(var=a\,b\, \,d\;e\;f\,g)

The escape characters, as expected, do not end up in the variable.

   -- Executing [954@dialstation:1] NoOp("SIP/x28-018ba000", "Generic test.  ") in new stack
   -- Executing [954@dialstation:2] Set("SIP/x28-018ba000", "var=a,b, ,d;e;f,g") in new stack
   -- Executing [954@dialstation:3] NoOp("SIP/x28-018ba000", "a,b, ,d;e;f,g") in new stack
   -- Executing [954@dialstation:4] NoOp("SIP/x28-018ba000", "b") in new stack
   -- Executing [954@dialstation:5] NoOp("SIP/x28-018ba000", "e") in new stack
   -- Executing [954@dialstation:6] NoOp("SIP/x28-018ba000", "onward....") in new stack

Getting this right is not all that simple, of course.  Consider the following:

The following is right:

   -- Executing [958@dialstation:5] Set("SIP/x28-0189aa00", "cid=Caller Name is Snom 28\, Number is 28") in new stack
   -- Executing [958@dialstation:6] AGI("SIP/x28-0189aa00", "sayexpnum,Caller Name is Snom 28\, Number is 28") in new stack

Because if the escape didn't stay in, you end up with the AGI thinking it has more than one argument.

Please consider how best to fix this globally, in the mean time, I think users can expect to have to continue adjusting dial plans until a permanent parsing scheme is adopted that then will hopefully never be incompatibly changed.

/john
Comments:By: Tilghman Lesher (tilghman) 2009-01-13 12:17:32.000-0600

Actually, we'd rather go with something a little more portable, like this.
By: John Covert (jcovert) 2009-01-13 13:13:59.000-0600

That works for system; however, you should allocate space for it dynamically (and free it), in case the System string has been built with the Set Command, which can build strings at least 4096 characters long.

And speaking of the Set command, it faces a similar problem.

I suppose you could create a variable with a ';' in it an concatentate it into
the variable.  As it stands now, you can only get '\;' in.

/john
By: Tilghman Lesher (tilghman) 2009-01-13 13:51:35.000-0600

While that may be true, I'm really not all that interested in longer strings for the System command.  Anything longer and it's probably better if you use multiple System commands.
By: Leif Madsen (lmadsen) 2009-02-02 14:12:39.000-0600

jcovert:  ping?  Any chance of getting a test on this?
By: John Covert (jcovert) 2009-02-05 17:33:51.000-0600

Sorry, will get to it by the weekend.  I see that the patch has now been rewritten to do the dynamic allocation I had suggested.  Thanks.
By: Leif Madsen (lmadsen) 2009-02-13 13:39:57.000-0600

Did you happen to get a chance to try this out last weekend John? Any chances of getting to it this weekend?
By: John Covert (jcovert) 2009-02-17 10:32:10.000-0600

Workin on it today.  Have just converted the patch to a 1.6.0.3 patch for testing.  Will report back later today.
By: John Covert (jcovert) 2009-02-17 19:16:50.000-0600

OK.  The new routine "ast_str_get_encoded_str" contained an error involving a pre-increment of the variable "offset" while using it as an insertion pointer which should have been a post-increment.  I have uploaded a corrected version of the patch as well as a version applicable to 1.6.0.3 (which is what I'm currently running).

I note that at trunk/head the related routine ast_get_encoded_str has been changed from an "int" always returning 0 to a "char *" returning "result".  The reviewers may suggest changing ast_str_get_encoded_str from an int always returning 0 to a "struct ast_str *" returning "*str".
By: Digium Subversion (svnbot) 2009-02-20 11:29:52.000-0600

Repository: asterisk
Revision: 177664

U   trunk/apps/app_system.c
U   trunk/include/asterisk/app.h
U   trunk/main/app.c

------------------------------------------------------------------------
r177664 | tilghman | 2009-02-20 11:29:51 -0600 (Fri, 20 Feb 2009) | 8 lines

Allow semicolons to be escaped, when passing arguments to the System command.
(closes issue ASTERISK-13358)
Reported by: jcovert
Patches:
      20090113__bug14231__2.diff.txt uploaded by Corydon76 (license 14)
      corrected_20090113__bug14231__2.diff.txt uploaded by jcovert (license 551)
Tested by: jcovert

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

http://svn.digium.com/view/asterisk?view=rev&revision=177664
By: Digium Subversion (svnbot) 2009-02-20 11:42:18.000-0600

Repository: asterisk
Revision: 177665

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_system.c
U   branches/1.6.0/include/asterisk/app.h
U   branches/1.6.0/include/asterisk/strings.h
U   branches/1.6.0/main/app.c

------------------------------------------------------------------------
r177665 | tilghman | 2009-02-20 11:42:17 -0600 (Fri, 20 Feb 2009) | 15 lines

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

........
 r177664 | tilghman | 2009-02-20 11:29:51 -0600 (Fri, 20 Feb 2009) | 8 lines
 
 Allow semicolons to be escaped, when passing arguments to the System command.
 (closes issue ASTERISK-13358)
  Reported by: jcovert
  Patches:
        20090113__bug14231__2.diff.txt uploaded by Corydon76 (license 14)
        corrected_20090113__bug14231__2.diff.txt uploaded by jcovert (license 551)
  Tested by: jcovert
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=177665
By: John Covert (jcovert) 2009-02-20 11:42:40.000-0600

The "pbx.c" patch that fixed Set to make it possible to put a ";" into a variable was not committed.
By: Tilghman Lesher (tilghman) 2009-02-20 12:17:33.000-0600

Correct, because it would have broken other things.
By: Digium Subversion (svnbot) 2009-02-20 15:35:18.000-0600

Repository: asterisk
Revision: 177761

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_system.c
U   branches/1.6.1/include/asterisk/app.h
U   branches/1.6.1/main/app.c

------------------------------------------------------------------------
r177761 | tilghman | 2009-02-20 15:35:18 -0600 (Fri, 20 Feb 2009) | 15 lines

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

........
 r177664 | tilghman | 2009-02-20 11:29:51 -0600 (Fri, 20 Feb 2009) | 8 lines
 
 Allow semicolons to be escaped, when passing arguments to the System command.
 (closes issue ASTERISK-13358)
  Reported by: jcovert
  Patches:
        20090113__bug14231__2.diff.txt uploaded by Corydon76 (license 14)
        corrected_20090113__bug14231__2.diff.txt uploaded by jcovert (license 551)
  Tested by: jcovert
........

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

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