From 7b994b6a7ef6472505f7aebde145cd609ff0a96a Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sat, 13 Jun 2020 05:12:10 -0700 Subject: [PATCH] Implement a better fix for unsetting special env vars The regression this commit fixes was first introduced in ksh93t 2008-07-25. It was previously worked around in 6f0e008c by forking subshells if any special environment variable is unset. The reason why this problem doesn't occur in ksh93s+ is because in that version of ksh sh_assignok never moves nodes, it only clones them. The second argument doesn't set NV_MOVE, which makes `sh_assignok(np,0)` is similar to `sh_assignok(np,1)`. In ksh93t and higher, setting the second argument to zero causes the node to be moved with NV_MOVE, which causes the discipline function associated with the variable node to be removed when `np->nvfun` is set to zero (i.e. NULL). This is why a command like `(unset LC_NUMERIC; LC_NUMERIC=invalid)` doesn't print a diagnostic, as it looses its discipline function. This patch fixes the problem by cloning the node with sh_assignok if it is a special variable with a discipline function. This allows special variables to work as expected in virtual subshells. The original workaround has been kept for the $PATH variable only, as hash tables are still broken in virtual subshells. It has been updated accordingly to only fork subshells if it detects the variable node for PATH. I have added two more regression tests for changing the PATH in subshells to make sure hash tables continue working as expected with this fix. src/cmd/ksh93/bltins/typeset.c: - Only fork virtual subshells if the PATH will be changed. If a variable is a special variable with a discipline function, it should be just be cloned, not moved. src/cmd/ksh93/sh/nvdisc.c: - Add a comment to clarify that NV_MOVE will delete the discipline function associated with the node. src/cmd/ksh93/tests/subshells.sh: - Add two more regression tests for unsetting the PATH in subshells, one for if PATH is being pointed to by a nameref. Condense the hash table tests by moving the main test into a single function. --- src/cmd/ksh93/bltins/typeset.c | 25 +++++++++++++++++-------- src/cmd/ksh93/sh/nvdisc.c | 2 +- src/cmd/ksh93/tests/subshell.sh | 16 +++++++++++----- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c index c1b1c0cf2410..79f833bb35a3 100644 --- a/src/cmd/ksh93/bltins/typeset.c +++ b/src/cmd/ksh93/bltins/typeset.c @@ -1208,11 +1208,9 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) } /* - * Check for variables with internal trap/discipline functions (PATH, LANG, LC_*, LINENO, etc.). - * Unsetting these in a virtual/non-forked subshell would cause them to lose their discipline actions, - * so, for example, (unset PATH; PATH=/dev/null; ls) would run 'ls'! Until a fix is found, make the problem - * go away by forking the subshell. To avoid crashing, this must be done before calling sh_pushcontext(), - * so we need to loop through the args separately to check if any variable to unset has a discipline function. + * Unsetting the PATH in a non-forked subshell will cause the parent shell's hash table to be reset, + * so fork to work around the problem. To avoid crashing, this must be done before calling sh_pushcontext(), + * so we need to loop through the args separately and check if any node is equal to PATHNOD. */ if(shp->subshell && !shp->subshare && troot==shp->var_tree) { @@ -1222,8 +1220,8 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) np=nv_open(name,troot,NV_NOADD|nflag); if(!np) continue; - /* Has discipline function, and is not a nameref? */ - if(np->nvfun && np->nvfun->disc && !nv_isref(np)) + /* Are we changing the PATH? */ + if(np==PATHNOD) { nv_close(np); sh_subfork(); @@ -1276,7 +1274,18 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) } if(shp->subshell) - np=sh_assignok(np,0); + { + if(!nv_isattr(np,NV_NODISC|NV_ARRAY) && !nv_isvtree(np)) + { + /* + * Variables with internal trap/discipline functions (LC_*, LINENO, etc.) need to be + * cloned, as moving them will remove the discipline function. + */ + np=sh_assignok(np,1); + } + else + np=sh_assignok(np,0); + } } if(!nv_isnull(np) || nv_size(np) || nv_isattr(np,~(NV_MINIMAL|NV_NOFREE))) diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c index db027fad8964..d054cc4e4b08 100644 --- a/src/cmd/ksh93/sh/nvdisc.c +++ b/src/cmd/ksh93/sh/nvdisc.c @@ -962,7 +962,7 @@ int nv_clone(Namval_t *np, Namval_t *mp, int flags) { if(nv_isattr(np,NV_INTEGER)) mp->nvalue.ip = np->nvalue.ip; - np->nvfun = 0; + np->nvfun = 0; /* This will remove the discipline function, if there is one */ np->nvalue.cp = 0; if(!nv_isattr(np,NV_MINIMAL) || nv_isattr(mp,NV_EXPORT)) { diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh index 3162f0699a1e..1a676f67d902 100755 --- a/src/cmd/ksh93/tests/subshell.sh +++ b/src/cmd/ksh93/tests/subshell.sh @@ -710,13 +710,19 @@ v=${ eval 'al'; alias al='echo subshare'; } && [[ $v == 'mainalias' && $(eval 'a || err_exit 'alias redefinition fails to survive ${ ...; }' # Resetting a subshell's hash table should not affect the parent shell -(hash -r) -[[ $(hash) ]] || err_exit $'resetting the hash table in a subshell affects the parent shell\'s hash table' -(PATH="$PATH") -[[ $(hash) ]] || err_exit $'resetting the PATH in a subshell affects the parent shell\'s hash table' +check_hash_table() +{ + [[ $(hash) ]] || err_exit $'resetting the hash table in a subshell affects the parent shell\'s hash table' + # Ensure the hash table isn't empty before the next test is run + hash -r chmod +} + +(hash -r); check_hash_table +(PATH="$PATH"); check_hash_table +(unset PATH); check_hash_table +(nameref PATH_TWO=PATH; unset PATH_TWO); check_hash_table # Adding a utility to a subshell's hash table should not affect the parent shell -hash -r chmod (hash cat) [[ $(hash) == "chmod=$(whence -p chmod)" ]] || err_exit $'changes to a subshell\'s hash table affect the parent shell'