From 2d61d2e5568d28edc671f19f798439f58d3a572d Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sat, 24 Sep 2022 07:56:03 -0700 Subject: [PATCH] Fix += operator in invocation-local scopes (re: 75796a9c) 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 2e816c32). 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 Resolves: https://github.com/ksh93/ksh/issues/533 --- NEWS | 4 +++ src/cmd/ksh93/include/shell.h | 1 + src/cmd/ksh93/sh/fault.c | 1 + src/cmd/ksh93/sh/name.c | 17 +++++++----- src/cmd/ksh93/sh/xec.c | 18 ++++++++++++ src/cmd/ksh93/tests/variables.sh | 47 ++++++++++++++++++++++++++++++++ 6 files changed, 81 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 7f0605bdd5d4..99bc10f17ef0 100644 --- a/NEWS +++ b/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, diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h index aea8cfd048d5..fd09acb21bb5 100644 --- a/src/cmd/ksh93/include/shell.h +++ b/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; diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c index 9226a2295fb3..98d6a7e5e1c5 100644 --- a/src/cmd/ksh93/sh/fault.c +++ b/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) diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c index 6f4c628271c3..61a2bedd8266 100644 --- a/src/cmd/ksh93/sh/name.c +++ b/src/cmd/ksh93/sh/name.c @@ -774,6 +774,7 @@ 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); @@ -781,7 +782,7 @@ Namval_t *nv_create(const char *name, Dt_t *root, int flags, Namfun_t *dp) 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,9 +791,9 @@ 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!='.') { @@ -800,17 +801,19 @@ Namval_t *nv_create(const char *name, Dt_t *root, int flags, Namfun_t *dp) 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) diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index ff418f2c5493..b76bceb1b9a6 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/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); } diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index ec194df1f516..1e780b64d263 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/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))