| Summary: | ASTERISK-15568: [patch] zero/empty argument to gosub yields callers $ARG1 | ||
| Reporter: | Walter Doekes (wdoekes) | Labels: | |
| Date Opened: | 2010-02-02 15:34:31.000-0600 | Date Closed: | 2010-03-18 14:39:43 |
| Priority: | Trivial | Regression? | No |
| Status: | Closed/Complete | Components: | Applications/app_stack |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) 20100202__issue16758.diff.txt ( 1) 20100316__issue16758.diff.txt ( 2) astsvn-16758-gosub-0-arguments.diff ( 3) astsvn-16758-gosub-0-arguments-2.diff ( 4) astsvn-16758-gosub-clean-args.diff | |
| Description: | Filed under app/app_stack as it's my most immediate problem, but it could be filed under extensions.conf-parsing as well. $ARG1..n behave like LOCAL() variables (I assume?). It's perfectly okay to me that when I do not supply an Nth argument to a procedure, I get the parents $ARGN instead. It is not okay, however, that my empty first argument is ignored. Two possible fixes: - clean all the $ARGN's before entering the new frame (frame_set_var for many arguments? expensive? not backward compatible) - parse a zero-length argument list as one argument, not zero (depending one where you do it, it could be okay) ****** STEPS TO REPRODUCE ****** Dialplan: [default] exten => s,1,NoOp(test) exten => s,n,Gosub(func_outer,s,1(some_argument,,third_argument)) [func_outer] exten => s,1,Set(LOCAL(empty_arg)=) exten => s,n,Gosub(func_inner,s,1(${empty_arg})) exten => s,n,Gosub(func_inner,s,1(${empty_arg},)) exten => s,n,Return() [func_inner] exten => s,1,NoOp(ARG1: "${ARG1}" "${ARG3}") exten => s,n,Return() Actual behaviour: -- Executing [s@default:1] NoOp("Local/s@default-4208;2", "test") in new stack -- Executing [s@default:2] Gosub("Local/s@default-4208;2", "func_outer,s,1(some_argument,,third_argument)") in new stack -- Executing [s@func_outer:1] Set("Local/s@default-4208;2", "LOCAL(empty_arg)=") in new stack -- Executing [s@func_outer:2] Gosub("Local/s@default-4208;2", "func_inner,s,1()") in new stack -- Executing [s@func_inner:1] NoOp("Local/s@default-4208;2", "ARG1: "some_argument" "third_argument"") in new stack <-- HERE -- Executing [s@func_inner:2] Return("Local/s@default-4208;2", "") in new stack -- Executing [s@func_outer:3] Gosub("Local/s@default-4208;2", "func_inner,s,1(,)") in new stack -- Executing [s@func_inner:1] NoOp("Local/s@default-4208;2", "ARG1: "" "third_argument"") in new stack -- Executing [s@func_inner:2] Return("Local/s@default-4208;2", "") in new stack -- Executing [s@func_outer:4] Return("Local/s@default-4208;2", "") in new stack Expected behaviour: -- Executing [s@default:1] NoOp("Local/s@default-2b3f;2", "test") in new stack -- Executing [s@default:2] Gosub("Local/s@default-2b3f;2", "func_outer,s,1(some_argument,,third_argument)") in new stack -- Executing [s@func_outer:1] Set("Local/s@default-2b3f;2", "LOCAL(empty_arg)=") in new stack -- Executing [s@func_outer:2] Gosub("Local/s@default-2b3f;2", "func_inner,s,1()") in new stack -- Executing [s@func_inner:1] NoOp("Local/s@default-2b3f;2", "ARG1: "" "third_argument"") in new stack <-- HERE -- Executing [s@func_inner:2] Return("Local/s@default-2b3f;2", "") in new stack -- Executing [s@func_outer:3] Gosub("Local/s@default-2b3f;2", "func_inner,s,1(,)") in new stack -- Executing [s@func_inner:1] NoOp("Local/s@default-2b3f;2", "ARG1: "" "third_argument"") in new stack -- Executing [s@func_inner:2] Return("Local/s@default-2b3f;2", "") in new stack -- Executing [s@func_outer:4] Return("Local/s@default-2b3f;2", "") in new stack ****** ADDITIONAL INFORMATION ****** The supplied patch 1 provides the expected behaviour by returning 1 argument from __ast_app_separate_args on buf=="". However, this would mean that apps taking zero arguments will now always get one. Furthermore, if you'd accept the change in functionality change, you'd probably want to reorder some things in __ast_app_separate_args. I bet you could do away with those final ifs. Patch 2 cheats a bit around the issue by fixing it for Gosub only. Not pretty, but probably safer. Regards, Walter Doekes | ||
| Comments: | By: Tilghman Lesher (tilghman) 2010-02-02 16:26:31.000-0600 I think what I'd rather do on this is to blank out all arguments previously on the stack. By: Walter Doekes (wdoekes) 2010-02-03 04:26:23.000-0600 That's probably better (more intuitive, less compatible). I don't see any reason to use ~ARGC~ over ARGC though: (1) no need to store max_argc. args2.argc is fine as we've cleaned up to max_argc already (2) both ~ARGC~ and ARGC are modifiable by the user, possibly yielding unexpected results. By: Walter Doekes (wdoekes) 2010-02-03 04:27:25.000-0600 (Bah.. ignore the diff in chan_sip.c.) By: Tilghman Lesher (tilghman) 2010-02-03 12:18:16.000-0600 1) It allows us to protect arguments at lower levels from modification. Think of a subroutine with multiple optional arguments, and I think you can see a scenario where that's beneficial. 2) That's true. I could possibly protect that variable, by encoding it in a datastore. By: Walter Doekes (wdoekes) 2010-02-04 02:35:51.000-0600 (1) Hm :) Yes, you're right. (2) It's probably not a problem. I believe the worst that can happen is that you get too few overwritten ARGN's or too many ($ARG1..<INT_MAX>). By: Tilghman Lesher (tilghman) 2010-03-16 17:42:41 Patch updated. The following notable changes: 1) Maximum number of arguments has been protected in a datastore, so it cannot be modified in the dialplan. 2) A unit test has been written to verify the behavior of the Gosub suite of applications. By: Digium Subversion (svnbot) 2010-03-16 18:49:36 Repository: asterisk Revision: 252976 U trunk/apps/app_stack.c A trunk/tests/test_gosub.c ------------------------------------------------------------------------ r252976 | tilghman | 2010-03-16 18:49:35 -0500 (Tue, 16 Mar 2010) | 8 lines Mask out previous arguments on each nested invocation of Gosub. (closes issue ASTERISK-15568) Reported by: wdoekes Patches: 20100316__issue16758.diff.txt uploaded by tilghman (license 14) Review: https://reviewboard.asterisk.org/r/561/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=252976 By: Digium Subversion (svnbot) 2010-03-16 18:53:51 Repository: asterisk Revision: 252977 _U branches/1.6.1/ U branches/1.6.1/apps/app_stack.c ------------------------------------------------------------------------ r252977 | tilghman | 2010-03-16 18:53:50 -0500 (Tue, 16 Mar 2010) | 15 lines Merged revisions 252976 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r252976 | tilghman | 2010-03-16 18:49:35 -0500 (Tue, 16 Mar 2010) | 8 lines Mask out previous arguments on each nested invocation of Gosub. (closes issue ASTERISK-15568) Reported by: wdoekes Patches: 20100316__issue16758.diff.txt uploaded by tilghman (license 14) Review: https://reviewboard.asterisk.org/r/561/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=252977 By: Digium Subversion (svnbot) 2010-03-16 18:54:31 Repository: asterisk Revision: 252978 _U branches/1.6.2/ U branches/1.6.2/apps/app_stack.c ------------------------------------------------------------------------ r252978 | tilghman | 2010-03-16 18:54:31 -0500 (Tue, 16 Mar 2010) | 15 lines Merged revisions 252976 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r252976 | tilghman | 2010-03-16 18:49:35 -0500 (Tue, 16 Mar 2010) | 8 lines Mask out previous arguments on each nested invocation of Gosub. (closes issue ASTERISK-15568) Reported by: wdoekes Patches: 20100316__issue16758.diff.txt uploaded by tilghman (license 14) Review: https://reviewboard.asterisk.org/r/561/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=252978 By: Digium Subversion (svnbot) 2010-03-16 18:54:44 Repository: asterisk Revision: 252979 _U branches/1.6.0/ U branches/1.6.0/apps/app_stack.c ------------------------------------------------------------------------ r252979 | tilghman | 2010-03-16 18:54:43 -0500 (Tue, 16 Mar 2010) | 15 lines Merged revisions 252976 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r252976 | tilghman | 2010-03-16 18:49:35 -0500 (Tue, 16 Mar 2010) | 8 lines Mask out previous arguments on each nested invocation of Gosub. (closes issue ASTERISK-15568) Reported by: wdoekes Patches: 20100316__issue16758.diff.txt uploaded by tilghman (license 14) Review: https://reviewboard.asterisk.org/r/561/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=252979 | ||