Fix 'unset -f' to work in subshells without forking (re: 047cb33)

This commit implements unsetting functions in virtual subshells,
removing the need for the forking workaround. This is done by
either invalidating the function found in the current subshell
function tree by unsetting its NV_FUNCTION attribute bits (which
will cause sh_exec() to skip it) or, if the function exists in a
parent shell, by creating an empty dummy subshell node in the
current function tree without that attribute.

As a beneficial side effect, it seems that bug 228 (unset -f fails
in forked subshells if a function is defined before forking) is now
also fixed.

src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/init.c:
- Add sh.fun_base for a saved pointer to the main shell's function
  tree for checking when in a subshell, analogous to sh.var_base.

src/cmd/ksh93/bltins/typeset.c: unall():
- Remove the fork workaround.
- When unsetting a function found in the current function tree
  (troot) and that tree is not sh.var_base (which checks if we're
  in a virtual subshell in a way that handles shared-state command
  substitutions correctly), then do not delete the function but
  invalidate it by unsetting its NV_FUNCTION attribute bits.
- When unsetting a function not found in the current function tree,
  search for it in sh.fun_base and if found, add an empty dummy
  node to mask the parent shell environment's function. The dummy
  node will not have NV_FUNCTION set, so sh_exec() will skip it.

src/cmd/ksh93/sh/subshell.c:
- sh_subfuntree(): For 'unset -f' to work correctly with
  shared-state command substitutions (subshares), this function
  needs a fix similar to the one applied to sh_assignok() for
  variables in commit 911d6b0. Walk up on the subshells tree until
  we find a non-subshare.
- sh_subtracktree(): Apply the same fix for the hash table.
- Remove table_unset() and incorporate an updated version of its
  code in sh_subshell(). As of ec88886, this function was only
  used to clean up the subshell function table as the alias table
  no longer exists.
- sh_subshell():
  * Simplify the loop to free the subshell hash table.
  * Add table_unset() code, slightly refactored for readability.
    Treat dummy nodes now created by unall() separately to avoid a
    memory leak; they must be nv_delete()d without passing the
    NV_FUNCTION bits. For non-dummy nodes, turn on the NV_FUNCTION
    attribute in case they were invalidated by unall(); this is
    needed for _nv_unset() to free the function definition.

src/cmd/ksh93/tests/subshell.sh:
- Update the test for multiple levels of subshell functions to test
  a subshare as well. While we're add it, add a very similar test
  for multiple levels of subshell variables that was missing.
- Add @JohnoKing's reproducer from #228.

src/cmd/ksh93/tests/leaks.sh:
- Add leak tests for unsetting functions in a virtual subshell.
  Test both the simple unset case (unall() creates a dummy node)
  and the define/unset case (unall() invalidates existing node).

Resolves: #228

src/cmd/ksh93/bltins/typeset.c

@@ -1277,12 +1277,6 @@ 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':
@@ -1388,33 +1382,24 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp)
 				_nv_unset(np,0);
 			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))
+			else if(isfun)
 			{
-				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 = troot;
-				while((troottmp = troottmp->view) && (np = nv_search(name,troottmp,0)) && is_afunction(np))
-					nv_delete(np,troottmp,0);
+				if(troot!=shp->fun_base)
+					nv_offattr(np,NV_FUNCTION);	/* invalidate */
+				else if(!(np->nvalue.rp && np->nvalue.rp->running))
+					nv_delete(np,troot,0);
 			}
 			/* The alias has been unset by call to _nv_unset, remove it from the tree */
 			else if(troot==shp->alias_tree)
 				nv_delete(np,troot,nofree_attr);
-#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))
-				nv_delete(np,shp->var_tree,0);
-#endif
 			else
 				nv_close(np);
 
 		}
 		else if(troot==shp->alias_tree)
 			r = 1;
+		else if(troot==shp->fun_tree && troot!=shp->fun_base && nv_search(name,shp->fun_base,0))
+			nv_open(name,troot,NV_NOSCOPE);	/* create dummy virtual subshell node without NV_FUNCTION attribute */
 	}
 	sh_popcontext(shp,&buff);
 	return(r);

src/cmd/ksh93/include/defs.h

@@ -165,6 +165,7 @@ struct shared
 	int		path_err;	/* last error on path search */ \
 	Dt_t		*track_tree;	/* for tracked aliases */ \
 	Dt_t		*var_base;	/* global level variables */ \
+	Dt_t		*fun_base;	/* global level functions */ \
 	Dt_t		*openmatch; \
 	Namval_t	*namespace;	/* current active namespace */ \
 	Namval_t	*last_table;	/* last table used in last nv_open */ \

src/cmd/ksh93/sh/init.c

@@ -1821,7 +1821,7 @@ static Init_t *nv_init(Shell_t *shp)
 	dtuserdata(shp->track_tree,shp,1);
 	shp->bltin_tree = sh_inittree(shp,(const struct shtable2*)shtab_builtins);
 	dtuserdata(shp->bltin_tree,shp,1);
-	shp->fun_tree = dtopen(&_Nvdisc,Dtoset);
+	shp->fun_base = shp->fun_tree = dtopen(&_Nvdisc,Dtoset);
 	dtuserdata(shp->fun_tree,shp,1);
 	dtview(shp->fun_tree,shp->bltin_tree);
 	nv_mount(DOTSHNOD, "type", shp->typedict=dtopen(&_Nvdisc,Dtoset));

src/cmd/ksh93/sh/subshell.c

@@ -426,12 +426,21 @@ static void nv_restore(struct subshell *sp)
 Dt_t *sh_subtracktree(int create)
 {
 	register struct subshell *sp = subshell_data;
-	if(create && sh.subshell && !sh.subshare && sp && !sp->strack)
+	if(create && sh.subshell)
 	{
-		sp->strack = dtopen(&_Nvdisc,Dtset);
-		dtuserdata(sp->strack,&sh,1);
-		dtview(sp->strack,sh.track_tree);
-		sh.track_tree = sp->strack;
+		if(sh.subshare)
+		{
+			while(sp->subshare)	/* move up as long as parent is also a ${ subshare; } */
+				sp = sp->prev;
+			sp = sp->prev;		/* move to first non-subshare parent */
+		}
+		if(sp && !sp->strack)
+		{
+			sp->strack = dtopen(&_Nvdisc,Dtset);
+			dtuserdata(sp->strack,&sh,1);
+			dtview(sp->strack,sh.track_tree);
+			sh.track_tree = sp->strack;
+		}
 	}
 	return(sh.track_tree);
 }
@@ -443,39 +452,23 @@ Dt_t *sh_subtracktree(int create)
 Dt_t *sh_subfuntree(int create)
 {
 	register struct subshell *sp = subshell_data;
-	if(create && sh.subshell && !sh.subshare && sp && !sp->sfun)
+	if(create && sh.subshell)
 	{
-		sp->sfun = dtopen(&_Nvdisc,Dtoset);
-		dtuserdata(sp->sfun,&sh,1);
-		dtview(sp->sfun,sh.fun_tree);
-		sh.fun_tree = sp->sfun;
-	}
-	return(sh.fun_tree);
-}
-
-/*
- * Remove and free a subshell table at *root after leaving a virtual subshell.
- * Pass 'fun' as nonzero when removing a subshell's shell functions.
- */
-static void table_unset(Shell_t *shp,register Dt_t *root,int fun)
-{
-	register Namval_t *np,*nq;
-	int flag;
-	for(np=(Namval_t*)dtfirst(root);np;np=nq)
-	{
-		nq = (Namval_t*)dtnext(root,np);
-		flag=0;
-		/* Check for autoloaded function; it must not be freed. */
-		if(fun && np->nvalue.rp && np->nvalue.rp->fname && shp->fpathdict
-		&& nv_search(np->nvalue.rp->fname,shp->fpathdict,0))
+		if(sh.subshare)
 		{
-			np->nvalue.rp->fdict = 0;
-			flag = NV_NOFREE;
+			while(sp->subshare)	/* move up as long as parent is also a ${ subshare; } */
+				sp = sp->prev;
+			sp = sp->prev;		/* move to first non-subshare parent */
+		}
+		if(sp && !sp->sfun)
+		{
+			sp->sfun = dtopen(&_Nvdisc,Dtoset);
+			dtuserdata(sp->sfun,&sh,1);
+			dtview(sp->sfun,sh.fun_tree);
+			sh.fun_tree = sp->sfun;
 		}
-		else
-			_nv_unset(np,NV_RDONLY);
-		nv_delete(np,root,flag|NV_FUNCTION);
 	}
+	return(sh.fun_tree);
 }
 
 int sh_subsavefd(register int fd)
@@ -808,30 +801,47 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 		/* Clean up subshell hash table. */
 		if(sp->strack)
 		{
-			Namval_t *np, *prev_np;
+			Namval_t *np, *next_np;
 			/* Detach this scope from the unified view. */
 			shp->track_tree = dtview(sp->strack,0);
-			/* Delete (free) all elements of the subshell hash table. To allow dtnext() to
-			   set the pointer to the next item, we have to delete one item behind the loop. */
-			prev_np = 0;
-			np = (Namval_t*)dtfirst(sp->strack);
-			while(np)
+			/* Free all elements of the subshell hash table. */
+			for(np = (Namval_t*)dtfirst(sp->strack); np; np = next_np)
 			{
-				if(prev_np)
-					nv_delete(prev_np,sp->strack,0);
-				prev_np = np;
-				np = (Namval_t*)dtnext(sp->strack,np);
+				next_np = (Namval_t*)dtnext(sp->strack,np);
+				nv_delete(np,sp->strack,0);
 			}
-			if(prev_np)
-				nv_delete(prev_np,sp->strack,0);
 			/* Close and free the table itself. */
 			dtclose(sp->strack);
 		}
 		/* Clean up subshell function table. */
 		if(sp->sfun)
 		{
+			Namval_t *np, *next_np;
+			/* Detach this scope from the unified view. */
 			shp->fun_tree = dtview(sp->sfun,0);
-			table_unset(shp,sp->sfun,1);
+			/* Free all elements of the subshell function table. */
+			for(np = (Namval_t*)dtfirst(sp->sfun); np; np = next_np)
+			{
+				next_np = (Namval_t*)dtnext(sp->sfun,np);
+				if(!np->nvalue.rp)
+				{
+					/* Dummy node created by unall() to mask parent shell function. */
+					nv_delete(np,sp->sfun,0);
+					continue;
+				}
+				nv_onattr(np,NV_FUNCTION);  /* in case invalidated by unall() */
+				if(np->nvalue.rp->fname && shp->fpathdict && nv_search(np->nvalue.rp->fname,shp->fpathdict,0))
+				{
+					/* Autoloaded function. It must not be freed. */
+					np->nvalue.rp->fdict = 0;
+					nv_delete(np,sp->sfun,NV_FUNCTION|NV_NOFREE);
+				}
+				else
+				{
+					_nv_unset(np,NV_RDONLY);
+					nv_delete(np,sp->sfun,NV_FUNCTION);
+				}
+			}
 			dtclose(sp->sfun);
 		}
 		n = shp->st.trapmax-savst.trapmax;

src/cmd/ksh93/tests/leaks.sh

@@ -195,6 +195,40 @@ after=$(getmem)
 err_exit_if_leak 'POSIX function defined in virtual subshell'
 typeset -f foo >/dev/null && err_exit 'POSIX function leaks out of subshell'
 
+# Unsetting a function in a virtual subshell
+
+function foo { echo bar; }
+before=$(getmem)
+for ((i=0; i < N; i++))
+do	(unset -f foo)
+done
+after=$(getmem)
+err_exit_if_leak 'ksh function unset in virtual subshell'
+typeset -f foo >/dev/null || err_exit 'ksh function unset in subshell was unset in main shell'
+
+foo() { echo bar; }
+before=$(getmem)
+for ((i=0; i < N; i++))
+do	(unset -f foo)
+done
+after=$(getmem)
+err_exit_if_leak 'POSIX function unset in virtual subshell'
+typeset -f foo >/dev/null || err_exit 'POSIX function unset in subshell was unset in main shell'
+
+before=$(getmem)
+for ((i=0; i < N; i++))
+do	(function foo { echo baz; }; unset -f foo)
+done
+after=$(getmem)
+err_exit_if_leak 'ksh function defined and unset in virtual subshell'
+
+before=$(getmem)
+for ((i=0; i < N; i++))
+do	(foo() { echo baz; }; unset -f foo)
+done
+after=$(getmem)
+err_exit_if_leak 'POSIX function defined and unset in virtual subshell'
+
 # ======
 # Sourcing a dot script in a virtual subshell
 

src/cmd/ksh93/tests/subshell.sh

@@ -656,6 +656,7 @@ v=$("$SHELL" -c '. "$1"' x "$a") && [[ $v == ok ]] || err_exit "fail: more fun 4
 # ...multiple levels of subshell
 func() { echo mainfunction; }
 v=$(
+	set +x
 	(
 		func() { echo sub1; }
 		(
@@ -664,20 +665,34 @@ v=$(
 				func() { echo sub3; }
 				func
 				PATH=/dev/null
-				unset -f func
+				dummy=${ dummy=${ dummy=${ dummy=${ unset -f func; }; }; }; };  # test subshare within subshell
 				func 2>/dev/null
 				(($? == 127)) && echo ok_nonexistent || echo fail_zombie
 			)
 			func
 		)
 		func
-	)
-	func
+	) 2>&1
+	func 2>&1
 )
 expect=$'sub3\nok_nonexistent\nsub2\nsub1\nmainfunction'
 [[ $v == "$expect" ]] \
 || err_exit "multi-level subshell function failure (expected $(printf %q "$expect"), got $(printf %q "$v"))"
 
+# ... https://github.com/ksh93/ksh/issues/228
+fail() {
+	echo 'Failure'
+}
+exp=Success
+got=$(
+	foo() { true; }			# Define function before forking
+	ulimit -t unlimited 2>/dev/null	# Fork the subshell
+	unset -f fail
+	PATH=/dev/null fail 2>/dev/null || echo "$exp"
+)
+[[ $got == "$exp" ]] || err_exit 'unset -f fails in forked subshells if a function is defined before forking' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 # Unsetting or redefining aliases within subshells
 
@@ -945,6 +960,29 @@ v=main
 (v=sub; (d=${ v=shared; }; [[ $v == shared ]]) ) || err_exit "shared comsub in nested subshell wrongly scoped (2)"
 [[ $v == main ]] || err_exit "shared comsub leaks out of subshell (7)"
 
+# ...multiple levels of subshell
+v=main
+got=$(
+	(
+		v=sub1
+		(
+			v=sub2
+			(
+				v=sub3
+				echo $v
+				dummy=${ dummy=${ dummy=${ dummy=${ unset v; }; }; }; };  # test subshare within subshell
+				[[ -n $v || -v v ]] && echo fail_zombie || echo ok_nonexistent
+			)
+			echo $v
+		)
+		echo $v
+	)
+	echo $v
+)
+exp=$'sub3\nok_nonexistent\nsub2\nsub1\nmain'
+[[ $got == "$exp" ]] || err_exit "multi-level subshell function failure" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 # After the memory leak patch for rhbz#982142, this minor regression was introduced:
 # if a backtick command substitution expanded an alias, an extra space was inserted.