Fix redefining & unsetting functions in subshells (BUG_FNSUBSH)

Functions can now be correctly redefined and unset in subshell
environments (such as ( ... ), $(command substitutions), etc).
Before this fix, attempts to do this were silently ignored (!!!),
causing the wrong code (i.e.: the function by the same name from
the parent shell environment) to be executed.

Redefining and unsetting functions within "shared" command
substitutions of the form '${ ...; }' is also fixed.

Prior discussion: https://github.com/att/ast/issues/73

src/cmd/ksh93/sh/parse.c:
- A fix from George Koelher (URL above). He writes:
  | The parser can set t->comnamp to the wrong function.
  | Suppose that the shell has executed
  |     foo() { echo WRONG; }
  | and is now parsing
  |     (foo() { echo ok; } && foo)
  | The parser was setting t->comnamp to the wrong foo. [This
  | fix] doesn't set t->comnamp unless it was a builtin. Now the
  | subshell can't call t->comnamp, so it looks for foo and finds
  | the ok foo in the subshell's function tree.

src/cmd/ksh93/bltins/typeset.c:
- Unsetting functions in a virtual/non-forked subshell still
  doesn't work: nv_open() fails to find the function. To work
  around this problem, make 'unset -f' fork the subshell into its
  own process with sh_subfork().
- The workaround exposed another bug: if we unset a function in a
  subshell tree that overrode a function by the same name in the
  main shell, then nv_delete() exposes the function from the main
  shell scope. Since 'unset -f' now always forks a subshell, the
  fix is to simply walk though troot's parent views and delete any
  such zombie functions as well. (Without this, the 4 'more fun'
  tests in tests/subshell.sh fail.)

src/cmd/ksh93/sh/subshell.c: sh_subfuntree():
- Fix function (re)definitions and unsetting in "shared" command
  substitutions of the form '${ commandlist; }' (i.e.: if
  sp->shp->subshare is true). Though internally this is a weird
  form of virtual subshell, the manual page says it does not
  execute in a subshell (meaning, all changes must survive it), so
  a subshell function tree must not be created for these.

src/cmd/ksh93/tests/subshell.sh:
- Add regression tests related to these bugfixes. Test unsetting
  and redefining a function in all three forms of virtual subshell.

(cherry picked from commit dde387825ab1bbd9f2eafc5dc38d5fd0bf9c3652)

NEWS

@@ -4,6 +4,20 @@ For full details, see the git log at:
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2020-05-29:
+
+- Fix BUG_FNSUBSH: functions can now be correctly redefined and unset in
+  subshell environments (such as ( ... ), $(command substitutions), etc).
+  Before this fix, this was silently ignored, causing the function by the
+  same name from the parent shell environment to be executed instead.
+	fn() { echo mainsh; }
+	(fn() { echo subsh; }; fn); fn
+  This now correctly outputs "subsh mainsh" instead of "mainsh mainsh".
+	ls() { echo "ls executed"; }
+	(unset -f ls; ls); ls
+  This now correctly lists your directory and then prints "ls executed",
+  instead of printing "ls executed" twice.
+
 2020-05-21:
 
 - Fix truncating of files with the combined redirections '<>;file' and

TODO

@@ -54,11 +54,6 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/
   not work within the command substitution, acting as if standard output is
   still closed.
 
-- BUG_FNSUBSH: Function definitions within subshells (including command
-  substitutions) are ignored if a function by the same name exists in the
-  main shell, so the wrong function is executed. 'unset -f' is also silently
-  ignored. This only applies to non-forked subshells.
-
 - BUG_IFSGLOBS: In glob pattern matching (as in case or parameter
   substitution with # and %), if IFS starts with ? or * and the "$*"
   parameter expansion inserts any IFS separator characters, those characters

src/cmd/ksh93/bltins/typeset.c

@@ -1171,6 +1171,12 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp)
 	while(r = optget(argv,name)) switch(r)
 	{
 		case 'f':
+			/*
+			 * Unsetting functions in a virtual/non-forked subshell doesn't work due to limitations
+			 * in the subshell function tree mechanism. Work around this by forking the subshell.
+			 */
+			if(shp->subshell && !shp->subshare)
+				sh_subfork();
 			troot = sh_subfuntree(1);
 			break;
 		case 'a':
@@ -1253,7 +1259,18 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp)
 			if(troot==shp->var_tree && shp->st.real_fun && (dp=shp->var_tree->walk) && dp==shp->st.real_fun->sdict)
 				nv_delete(np,dp,NV_NOFREE);
 			else if(isfun && !(np->nvalue.rp && np->nvalue.rp->running))
+			{
 				nv_delete(np,troot,0);
+				/*
+				 * If we have just unset a function in a subshell tree that overrode a function by the same
+				 * name in the main shell, then the above nv_delete() call incorrectly restores the function
+				 * from the main shell scope. So walk though troot's parent views and delete any such zombie
+				 * functions. Note that this only works because 'unset -f' now forks if we're in a subshell.
+				 */
+				Dt_t *troottmp;
+				while((troottmp = troot->view) && (np = nv_search(name,troottmp,0)) && is_afunction(np))
+					nv_delete(np,troottmp,0);
+			}
 #if 0
 			/* causes unsetting local variable to expose global */
 			else if(shp->var_tree==troot && shp->var_tree!=shp->var_base && nv_search((char*)np,shp->var_tree,HASH_BUCKET|HASH_NOSCOPE))

src/cmd/ksh93/include/version.h

@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-05-21"
+#define SH_RELEASE	"93u+m 2020-05-29"

src/cmd/ksh93/sh/parse.c

@@ -1465,10 +1465,10 @@ static Shnode_t *simple(Lex_t *lexp,int flag, struct ionod *io)
 			{
 				/* check for builtin command */
 				Namval_t *np=nv_bfsearch(argp->argval,lexp->sh->fun_tree, (Namval_t**)&t->comnamq,(char**)0);
-				if(cmdarg==0)
-					t->comnamp = (void*)np;
 				if(np && is_abuiltin(np))
 				{
+					if(cmdarg==0)
+						t->comnamp = (void*)np;
 					if(nv_isattr(np,BLT_DCL))
 					{
 						assignment = 1+(*argp->argval=='a');

src/cmd/ksh93/sh/subshell.c

@@ -402,7 +402,7 @@ Dt_t *sh_subfuntree(int create)
 	register struct subshell *sp = subshell_data;
 	if(!sp || sp->shp->curenv==0)
 		return(sh.fun_tree);
-	if(!sp->sfun && create)
+	if(!sp->sfun && create && !sp->shp->subshare)
 	{
 		sp->sfun = dtopen(&_Nvdisc,Dtoset);
 		dtview(sp->sfun,sp->shp->fun_tree);

src/cmd/ksh93/tests/subshell.sh

@@ -623,4 +623,34 @@ do	if	[[ -e $f ]]
 	fi
 done
 
+# ======
+# Unsetting or redefining functions within subshells
+
+# ...function can be unset in subshell
+
+func() { echo mainfunction; }
+(unset -f func; typeset -f func >/dev/null 2>&1) && err_exit 'function fails to be unset in subshell'
+v=$(unset -f func; typeset -f func >/dev/null 2>&1) && err_exit 'function fails to be unset in comsub'
+v=${ unset -f func 2>&1; } && ! typeset -f func >/dev/null 2>&1 || err_exit 'function unset fails to survive ${ ...; }'
+
+# ...function can be redefined in subshell
+func() { echo mainfunction; }
+(func() { echo sub; }; [[ ${ func; } == sub ]]) || err_exit 'function fails to be redefined in subshell'
+v=$(func() { echo sub; }; func) && [[ $v == sub ]] || err_exit 'function fails to be redefined in comsub'
+v=${ { func() { echo sub; }; } 2>&1; } && [[ $(PATH=/dev/null func) == sub ]] \
+	|| err_exit 'function redefine fails to survive ${ ...; }'
+
+# ...some more fun from Stéphane Chazelas: https://github.com/att/ast/issues/73#issuecomment-384178095
+a=$tmp/morefun.sh
+cat >| "$a" <<EOF
+true() { echo WRONG; }
+(true() { echo ok; } && true && unset -f true && true) || false
+# the extra '|| false' avoids optimising out the subshell
+EOF
+v=$("$SHELL" "$a") && [[ $v == ok ]] || err_exit 'fail: more fun 1'
+v=$("$SHELL" -c "$(cat "$a")") && [[ $v == ok ]] || err_exit 'fail: more fun 2'
+v=$("$SHELL" -c 'eval "$(cat "$1")"' x "$a") && [[ $v == ok ]] || err_exit "fail: more fun 3"
+v=$("$SHELL" -c '. "$1"' x "$a") && [[ $v == ok ]] || err_exit "fail: more fun 4"
+
+# ======
 exit $((Errors<125?Errors:125))