Fix += operator in invocation-local scopes (re: 75796a9)

This commit fixes multiple related bugs in invocation-local scopes
when using the += operator. Changes:

- In sh_exec and sh_ntfork(), increment sh.invoc_local when
  entering an invocation-local scope, and decrement it when exiting
  the local scope. The nv_putval() function makes use of this
  variable when copying the parent scope variable into the local
  scope, which avoids setting the wrong scope for independent foo+=
  statements.

- To avoid possible function scoping bugs, reset sh.invoc_local in
  function scopes created with sh_funscope(). Set sh.invoc_local
  back to its previous value when exiting the function's static
  scope.

- In nv_create(), fix a regression introduced in ksh93t-
  2008-06-02. The regression causes nv_search to use the wrong
  scope, because nv_search doesn't use the NV_NOSCOPE flag
  (formerly HASH_NOSCOPE, see 2e816c3). As a result, nv_search
  will not find the locally scoped variable (since nv_putval has
  yet to create it) and searches the parent scope. This results in
  the variable node pointing to the parent scope's variable,
  resulting in the variable no longer having a local scope. To
  avoid this problem without introducing regression test failures,
  the resulting node found by nv_search is initially set to an mp
  pointer. The np pointer will only be set to mp (the result from
  nv_search) when needed (most of the time, this isn't in an
  invocation-local scope).

Co-authored-by: Martijn Dekker <martijn@inlv.org>
Resolves: #533

NEWS

@@ -7,6 +7,10 @@ Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
 - Fixed a bug introduced on 2021-04-04 that incorrectly allowed 'typeset' to
   turn off the readonly and export attributes on a readonly variable.
 
+- Fixed multiple scoping-related bugs in the += additive assignment operator,
+  including a bug introduced on 2021-04-11 that caused it to append to the
+  value of a global variable when used on a local variable with the same name.
+
 2022-09-24:
 
 - The down arrow key can now be used to perform a backwards reverse search,

src/cmd/ksh93/include/shell.h

@@ -337,6 +337,7 @@ struct Shell_s
 	struct dolnod	*arglist;
 	int16_t		fn_depth;	/* scoped ksh-style function call depth */
 	int16_t		dot_depth;	/* dot-script and POSIX function call depth */
+	char		invoc_local;	/* set when inside of an invocation-local scope */
 	int		hist_depth;
 	int		xargmin;
 	int		xargmax;

src/cmd/ksh93/sh/fault.c

@@ -630,6 +630,7 @@ void sh_exit(register int xno)
 	sh.intrace = 0;
 	sh.prefix = 0;
 	sh.mktype = 0;
+	sh.invoc_local = 0;
 	if(job.in_critical)
 		job_unlock();
 	if(pp->mode == SH_JMPSCRIPT && !pp->prev) 

src/cmd/ksh93/sh/name.c

@@ -774,14 +774,15 @@ Namval_t *nv_create(const char *name,  Dt_t *root, int flags, Namfun_t *dp)
 			if(top)
 			{
 				struct Ufunction *rp;
+				Namval_t *mp;
 				if((rp=sh.st.real_fun) && !rp->sdict && (flags&NV_STATIC))
 				{
 					Dt_t *dp = dtview(sh.var_tree,(Dt_t*)0);
 					rp->sdict = dtopen(&_Nvdisc,Dtoset);
 					dtview(rp->sdict,dp);
 					dtview(sh.var_tree,rp->sdict);
 				}
-				if(np = nv_search(name,sh.var_tree,0))
+				if(mp = nv_search(name,sh.var_tree,0))
 				{
 #if SHOPT_NAMESPACE
 					if(sh.var_tree->walk==sh.var_base || (sh.var_tree->walk!=sh.var_tree && sh.namespace &&  nv_dict(sh.namespace)==sh.var_tree->walk))
@@ -790,27 +791,29 @@ Namval_t *nv_create(const char *name,  Dt_t *root, int flags, Namfun_t *dp)
 #endif /* SHOPT_NAMESPACE */
 					{
 #if SHOPT_NAMESPACE
-						if(!(nq = nv_search((char*)np,sh.var_base,NV_REF)))
+						if(!(nq = nv_search((char*)mp,sh.var_base,NV_REF)))
 #endif /* SHOPT_NAMESPACE */
-						nq = np;
+						nq = mp;
 						sh.last_root = sh.var_tree->walk;
 						if((flags&NV_NOSCOPE) && *cp!='.')
 						{
 							if(mode==0)
 								root = sh.var_tree->walk;
 							else
 							{
-								nv_delete(np,(Dt_t*)0,NV_NOFREE);
-								np = 0;
+								nv_delete(mp,(Dt_t*)0,NV_NOFREE);
+								mp = 0;
 							}
 						}
+						np = mp;
 					}
-					else
+					else if(!sh.invoc_local)
 					{
 						if(sh.var_tree->walk)
 							root = sh.var_tree->walk;
 						flags |= NV_NOSCOPE;
 						noscope = 1;
+						np = mp;
 					}
 				}
 				if(rp && rp->sdict && (flags&NV_STATIC))
@@ -1561,7 +1564,7 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
 	 * When we are in an invocation-local scope and we are using the
 	 * += operator, clone the variable from the previous scope.
 	 */
-	if((flags&NV_APPEND) && nv_isnull(np) && sh.var_tree->view)
+	if(sh.invoc_local && (flags&NV_APPEND) && nv_isnull(np))
 	{
 		Namval_t *mp = nv_search(np->nvname,sh.var_tree->view,0);
 		if(mp)

src/cmd/ksh93/sh/xec.c

@@ -1241,6 +1241,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 						if(argp)
 						{
 							scope++;
+							sh.invoc_local++;
 							sh_scope(argp,0);
 						}
 						opt_info.index = opt_info.offset = 0;
@@ -1317,7 +1318,10 @@ int sh_exec(register const Shnode_t *t, int flags)
 					if(buffp->olist)
 						free_list(buffp->olist);
 					if(scope)
+					{
 						sh_unscope();
+						sh.invoc_local--;
+					}
 					bp->ptr = (void*)save_ptr;
 					bp->data = (void*)save_data;
 					sh.redir0 = 0;
@@ -1336,6 +1340,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 				if(!command && np && nv_isattr(np,NV_FUNCTION))
 				{
 					volatile int indx;
+					volatile char scope = 0;
 					struct checkpt *buffp = (struct checkpt*)stkalloc(sh.stk,sizeof(struct checkpt));
 #if SHOPT_NAMESPACE
 					Namval_t node, *namespace=0;
@@ -1393,6 +1398,11 @@ int sh_exec(register const Shnode_t *t, int flags)
 					}
 					if(jmpval == 0)
 					{
+						if(argp)
+						{
+							sh.invoc_local++;
+							scope++;
+						}
 						if(io)
 							indx = sh_redirect(io,execflg);
 #if SHOPT_NAMESPACE
@@ -1420,6 +1430,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 						sh_popcontext(buffp);
 						sh_iorestore(indx,jmpval);
 					}
+					if(scope)
+						sh.invoc_local--;
 					if(nq)
 						unset_instance(nq,&node,&nr,mode);
 					sh_funstaks(slp->slchild,-1);
@@ -3000,6 +3012,7 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 	int			isig,jmpval;
 	volatile int		r = 0;
 	int			n;
+	char			save_invoc_local;
 	char 			**savsig, *save_debugtrap = 0;
 	struct funenv		*fp = 0;
 	struct checkpt	*buffp = (struct checkpt*)stkalloc(sh.stk,sizeof(struct checkpt));
@@ -3080,6 +3093,8 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 		nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE);
 		nv_putval(SH_FUNNAMENOD,sh.st.funname,NV_NOFREE);
 	}
+	save_invoc_local = sh.invoc_local;
+	sh.invoc_local = 0;
 	jmpval = sigsetjmp(buffp->buff,0);
 	if(jmpval == 0)
 	{
@@ -3114,6 +3129,7 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 			r = sh.exitval;
 		}
 	}
+	sh.invoc_local = save_invoc_local;
 	sh.fn_depth--;
 	update_sh_level();
 	if(sh.fn_depth==1 && jmpval==SH_JMPERRFN)
@@ -3370,6 +3386,7 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd)
 		if(t->com.comset)
 		{
 			scope++;
+			sh.invoc_local++;
 			sh_scope(t->com.comset,0);
 		}
 		if(!strchr(path=argv[0],'/')) 
@@ -3512,6 +3529,7 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int topfd)
 	if(scope)
 	{
 		sh_unscope();
+		sh.invoc_local--;
 		if(jmpval==SH_JMPSCRIPT)
 			nv_setlist(t->com.comset,NV_EXPORT|NV_IDENT|NV_ASSIGN,0);
 	}

src/cmd/ksh93/tests/variables.sh

@@ -1476,5 +1476,52 @@ got=$("$SHELL" <<-\EOF
 ((exp == got)) || err_exit 'LINENO is wrong after a multi-line compound assignment' \
 	"(expected $exp, got $(printf %q "$got"))"
 
+# ======
+# The += operator shouldn't copy variables outside of a function's scope
+# https://github.com/ksh93/ksh/issues/533
+unset v foo bar
+v=outside
+function f {
+	typeset v
+	print -n "$v"
+	v+="inside"
+	print "$v"
+}
+function foo {
+	echo $bar
+}
+function bar {
+	bar=bar_
+	bar+=foo foo
+	bar+=foo "$SHELL" -c 'echo $bar'
+	bar+=foo
+	echo $bar
+}
+exp='inside
+bar_foo
+bar_foo
+bar_foo'
+got=$(f && bar)
+[[ $exp == "$got" ]] || err_exit "+= operator used in function copies variable from outside of the function's scope" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+unset var
+function three {
+        :
+}
+function two {
+	var+='wrong ' sh -c 'true'
+	var+='wrong ' true
+	var+='wrong ' three
+	echo $var
+}
+function one {
+	var=one_ two
+}
+exp=one_
+got=$(one)
+[[ $exp == "$got" ]] || err_exit "+= operator in a nested function appends variable in the wrong scope" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))