From 6f0e008cf7f9d7a6dc4316ac7dddb8eb4212ecc2 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Fri, 5 Jun 2020 15:31:26 +0200 Subject: [PATCH] Fix unsetting special vars in subshells (re: efa31503, 8b7f8f9b) This fixes (or at least works around) a bug that caused special variables such as PATH, LANG, LC_ALL, LINENO, etc. to lose their effect after being unset in a subshell. For example: (unset PATH; PATH=/dev/null; ls); : wrongly ran 'ls' (unset LC_ALL; LC_ALL=badlocale); : failed to print a diagnostic This is yet another problem with non-forking/virtual subshells. If you forced the subshell to fork (one way of doing this is using the 'ulimit' builtin, e.g. ulimit -t unlimited) before unsetting the special variable, the problem vanished. I've tried to localise the problem. I suspect the sh_assignok() function, which is called from unall(), is to blame. This function is supposed to make a copy of a variable node in the virtual subshell's variable tree. Apparently, it fails to copy the associated permanent discipline function settings (stored in the np->nvfun->disc pointer) that gave these variables their special effect, and which survive unset. However, my attempts to fix that have been unsuccessful. If anyone can figure out a fix, please send a patch/pull request! Data point: This bug existed in 93u 2011-02-08, but did not yet exist in M-1993-12-28-s+. So it is a regression. Meanwhile, pending a proper fix, this commit adds a safe workaround: it forces a non-forked subshell to fork before unsetting such a special variable. src/cmd/ksh93/bltins/typeset.c: unall(): - If we're in a non-forked, non-${ ...; } subshell, then before unsetting any variable, check for variables with internal trap/discipline functions, and call sh_subfork() if any are found. To avoid crashing, this must be done before calling sh_pushcontext(), so we need to loop through the args separately. src/cmd/ksh93/tests/variables.sh: - Remove the 'ulimit' that forced the fork; we do this in C now. (cherry picked from commit 21b1a67156582e3cbd36936f4af908bb45211a4b) --- NEWS | 7 +++++++ src/cmd/ksh93/bltins/typeset.c | 27 +++++++++++++++++++++++++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/tests/variables.sh | 3 --- 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 6eb121f68..81c4cbcdf 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,13 @@ For full details, see the git log at: Any uppercase BUG_* names are modernish shell bug IDs. +2020-06-05: + +- Fix a bug that caused special variables such as PATH, LANG, LC_ALL, + etc. to lose their effect after being unset in a subshell. For example: + (unset PATH; PATH=/dev/null; ls); : wrongly ran 'ls' + (unset LC_ALL; LC_ALL=badlocale); : failed to print a diagnostic + 2020-06-04: - Fix BUG_KBGPID: the $! special parameter was not set if a background job diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c index 276490350..99e7b0517 100644 --- a/src/cmd/ksh93/bltins/typeset.c +++ b/src/cmd/ksh93/bltins/typeset.c @@ -1209,6 +1209,33 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) dtclear(troot); return(r); } + + /* + * 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. + */ + if(shp->subshell && !shp->subshare && troot==shp->var_tree) + { + char **argv_tmp = argv; + while(name = *argv_tmp++) + { + 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)) + { + nv_close(np); + sh_subfork(); + break; + } + nv_close(np); + } + } + sh_pushcontext(shp,&buff,1); while(name = *argv++) { diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index be7f88eba..b8faf4c5a 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-06-04" +#define SH_RELEASE "93u+m 2020-06-05" diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index 34f4899df..f82cda7fd 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -642,9 +642,6 @@ set -- fi done - # TODO: changing the locale is somehow broken in non-forked/virtual subshells. - # For now, fork it using ulimit; remove the ulimit to expose the test failures. - ulimit -t unlimited x=x for v in LC_ALL LC_CTYPE LC_MESSAGES LC_COLLATE LC_NUMERIC do nameref r=$v