From 75796a9c75cacdc4001b686ef245c23ee68ae063 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sun, 11 Apr 2021 17:24:33 -0700 Subject: [PATCH] Fix += operator regressions (re: fae8862c) (#270) The bugfix for BUG_CMDSPASGN backported in commit fae8862c caused two regressions with the += operator: 1. The += operator did not append to variables. Reproducer: $ integer foo=3 $ foo+=2 command eval 'echo $foo' 2 2. The += operator ignored the readonly attribute, modifying readonly variables in the same manner as above. Reproducer $ readonly bar=str $ bar+=ing command eval 'echo $bar' ing Both of the regressions above were caused by nv_putval() failing to clone the variable from the previous scope into the invocation-local scope. As a result, 'foo+=2' was effectively 0 + 2 (since ksh didn't clone 3). The first regression was noticed during the development of ksh93v-, so to fix both bugs I've backported the bugfix for the regression from the ksh93v- 2013-10-10 alpha version: https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html src/cmd/ksh93/sh/name.c: - To fix both of the bugs above, find the variable to modify with nv_search(), then clone it into the invocation local scope. To fix the readonly bug as well, this is done before the NV_RDONLY check (otherwise np will be missing that attribute and be incorrectly modified in the invocation-local scope). - Update a nearby comment describing what sh_assignok() does (per this comment: https://github.com/ksh93/ksh/pull/249#issuecomment-811381759) src/cmd/ksh93/tests/builtins.sh: - Add regression tests for both of the now fixed regressions, loosely based on the regression tests in ksh93v-. --- NEWS | 11 +++++++++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/name.c | 18 +++++++++++++----- src/cmd/ksh93/tests/builtins.sh | 29 ++++++++++++++++++++++++++++- 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 27f9c6c6b..e255bbcbd 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,17 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2021-04-11: + +- Fixed two related regressions introduced on 2020-06-16: + 1. The += assignment failed to append the value of variables when used + in an invocation-local scope. The following should print '5', but + the regression resulted in '3' being printed instead: + $ integer foo=2; foo+=3 command eval 'echo $foo' + 3 + 2. Any += assignment used in an invocation-local scope could modify + readonly variables. + 2021-04-10: - Fixed: the internal count of the recursion level for arithmetic expressions diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 35529d808..98b87ce21 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -20,7 +20,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-alpha" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2021-04-10" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2021-04-11" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c index e4dec7d5a..01a969016 100644 --- a/src/cmd/ksh93/sh/name.c +++ b/src/cmd/ksh93/sh/name.c @@ -1601,15 +1601,23 @@ void nv_putval(register Namval_t *np, const char *string, int flags) #if SHOPT_FIXEDARRAY Namarr_t *ap; #endif /* SHOPT_FIXEDARRAY */ - if(!(flags&NV_RDONLY) && nv_isattr (np, NV_RDONLY)) + /* + * 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) && shp->var_tree->view) + { + Namval_t *mp = nv_search(np->nvname,shp->var_tree->view,0); + if(mp) + nv_clone(mp,np,0); + } + /* Don't alter readonly variables */ + if(!(flags&NV_RDONLY) && nv_isattr(np,NV_RDONLY)) { errormsg(SH_DICT,ERROR_exit(1),e_readonly, nv_name(np)); UNREACHABLE(); } - /* - * The following could cause the shell to fork if assignment - * would cause a side effect - */ + /* Create a local scope when inside of a virtual subshell */ shp->argaddr = 0; if(shp->subshell && !nv_local && !(flags&NV_RDONLY)) np = sh_assignok(np,1); diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh index b0b1bd966..56db9effb 100755 --- a/src/cmd/ksh93/tests/builtins.sh +++ b/src/cmd/ksh93/tests/builtins.sh @@ -740,10 +740,37 @@ printf '\\\000' | read -r -d '' [[ $REPLY == $'\\' ]] || err_exit "read -r -d '' ignores -r" # ====== -# Preceding a special builtin with 'command' should disable its special properties +# BUG_CMDSPASGN: Preceding a special builtin with 'command' should disable its special properties. foo=BUG command eval ':' [[ $foo == BUG ]] && err_exit "'command' fails to disable the special properties of special builtins" +# Regression that occurred after fixing the bug above: the += operator didn't work correctly. +# https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html +unset foo +integer foo=1 +exp=4 +got=$(foo+=3 command eval 'echo $foo') +[[ $exp == $got ]] || err_exit "[1]: += assignment for environment variables doesn't work with 'command special_builtin'" \ + "(expected $exp, got $got)" +foo+=3 command eval 'test $foo' +(( foo == 1 )) || err_exit "environment isn't restored after 'command special_builtin'" \ + "(expected 1, got $foo)" +got=$(foo+=3 eval 'echo $foo') +[[ $exp == $got ]] || err_exit "+= assignment for environment variables doesn't work with builtins" \ + "(expected $exp, got $got)" + +unset foo +exp=barbaz +got=$(foo=bar; foo+=baz command eval 'echo $foo') +[[ $exp == $got ]] || err_exit "[2]: += assignment for environment variables doesn't work with 'command special_builtin'" \ + "(expected $exp, got $got)" + +# Attempting to modify a readonly variable with the += operator should fail +exp=2 +got=$(integer -r foo=2; foo+=3 command eval 'echo $foo' 2> /dev/null) +[[ $? == 0 ]] && err_exit "+= assignment modifies readonly variables" \ + "(expected $exp, got $got)" + # ====== # 'whence -f' should ignore functions foo_bar() { true; }