From 047cb3303c7ea80a9cfde74c517b0496093abb65 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Fri, 29 May 2020 08:27:20 +0100 Subject: [PATCH] Fix redefining & unsetting functions in subshells (BUG_FNSUBSH) Functions can now be correctly redefined and unset in subshell environments (such as ( ... ), $(command substitutions), etc). Before this fix, attempts to do this were silently ignored (!!!), causing the wrong code (i.e.: the function by the same name from the parent shell environment) to be executed. Redefining and unsetting functions within "shared" command substitutions of the form '${ ...; }' is also fixed. Prior discussion: https://github.com/att/ast/issues/73 src/cmd/ksh93/sh/parse.c: - A fix from George Koelher (URL above). He writes: | The parser can set t->comnamp to the wrong function. | Suppose that the shell has executed | foo() { echo WRONG; } | and is now parsing | (foo() { echo ok; } && foo) | The parser was setting t->comnamp to the wrong foo. [This | fix] doesn't set t->comnamp unless it was a builtin. Now the | subshell can't call t->comnamp, so it looks for foo and finds | the ok foo in the subshell's function tree. src/cmd/ksh93/bltins/typeset.c: - Unsetting functions in a virtual/non-forked subshell still doesn't work: nv_open() fails to find the function. To work around this problem, make 'unset -f' fork the subshell into its own process with sh_subfork(). - The workaround exposed another bug: if we unset a function in a subshell tree that overrode a function by the same name in the main shell, then nv_delete() exposes the function from the main shell scope. Since 'unset -f' now always forks a subshell, the fix is to simply walk though troot's parent views and delete any such zombie functions as well. (Without this, the 4 'more fun' tests in tests/subshell.sh fail.) src/cmd/ksh93/sh/subshell.c: sh_subfuntree(): - Fix function (re)definitions and unsetting in "shared" command substitutions of the form '${ commandlist; }' (i.e.: if sp->shp->subshare is true). Though internally this is a weird form of virtual subshell, the manual page says it does not execute in a subshell (meaning, all changes must survive it), so a subshell function tree must not be created for these. src/cmd/ksh93/tests/subshell.sh: - Add regression tests related to these bugfixes. Test unsetting and redefining a function in all three forms of virtual subshell. (cherry picked from commit dde387825ab1bbd9f2eafc5dc38d5fd0bf9c3652) --- NEWS | 14 ++++++++++++++ TODO | 5 ----- src/cmd/ksh93/bltins/typeset.c | 17 +++++++++++++++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/parse.c | 4 ++-- src/cmd/ksh93/sh/subshell.c | 2 +- src/cmd/ksh93/tests/subshell.sh | 30 ++++++++++++++++++++++++++++++ 7 files changed, 65 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index a962ec18c..bda9d6be7 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,20 @@ For full details, see the git log at: Any uppercase BUG_* names are modernish shell bug IDs. +2020-05-29: + +- Fix BUG_FNSUBSH: functions can now be correctly redefined and unset in + subshell environments (such as ( ... ), $(command substitutions), etc). + Before this fix, this was silently ignored, causing the function by the + same name from the parent shell environment to be executed instead. + fn() { echo mainsh; } + (fn() { echo subsh; }; fn); fn + This now correctly outputs "subsh mainsh" instead of "mainsh mainsh". + ls() { echo "ls executed"; } + (unset -f ls; ls); ls + This now correctly lists your directory and then prints "ls executed", + instead of printing "ls executed" twice. + 2020-05-21: - Fix truncating of files with the combined redirections '<>;file' and diff --git a/TODO b/TODO index e7df53400..c8dd5a371 100644 --- a/TODO +++ b/TODO @@ -54,11 +54,6 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/ not work within the command substitution, acting as if standard output is still closed. -- BUG_FNSUBSH: Function definitions within subshells (including command - substitutions) are ignored if a function by the same name exists in the - main shell, so the wrong function is executed. 'unset -f' is also silently - ignored. This only applies to non-forked subshells. - - BUG_IFSGLOBS: In glob pattern matching (as in case or parameter substitution with # and %), if IFS starts with ? or * and the "$*" parameter expansion inserts any IFS separator characters, those characters diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c index 7a32c7043..aaaf0e7c5 100644 --- a/src/cmd/ksh93/bltins/typeset.c +++ b/src/cmd/ksh93/bltins/typeset.c @@ -1171,6 +1171,12 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) while(r = optget(argv,name)) switch(r) { case 'f': + /* + * Unsetting functions in a virtual/non-forked subshell doesn't work due to limitations + * in the subshell function tree mechanism. Work around this by forking the subshell. + */ + if(shp->subshell && !shp->subshare) + sh_subfork(); troot = sh_subfuntree(1); break; case 'a': @@ -1253,7 +1259,18 @@ static int unall(int argc, char **argv, register Dt_t *troot, Shell_t* shp) if(troot==shp->var_tree && shp->st.real_fun && (dp=shp->var_tree->walk) && dp==shp->st.real_fun->sdict) nv_delete(np,dp,NV_NOFREE); else if(isfun && !(np->nvalue.rp && np->nvalue.rp->running)) + { nv_delete(np,troot,0); + /* + * If we have just unset a function in a subshell tree that overrode a function by the same + * name in the main shell, then the above nv_delete() call incorrectly restores the function + * from the main shell scope. So walk though troot's parent views and delete any such zombie + * functions. Note that this only works because 'unset -f' now forks if we're in a subshell. + */ + Dt_t *troottmp; + while((troottmp = troot->view) && (np = nv_search(name,troottmp,0)) && is_afunction(np)) + nv_delete(np,troottmp,0); + } #if 0 /* causes unsetting local variable to expose global */ else if(shp->var_tree==troot && shp->var_tree!=shp->var_base && nv_search((char*)np,shp->var_tree,HASH_BUCKET|HASH_NOSCOPE)) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 4f7ec02d4..c03d5ee3e 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-05-21" +#define SH_RELEASE "93u+m 2020-05-29" diff --git a/src/cmd/ksh93/sh/parse.c b/src/cmd/ksh93/sh/parse.c index 64ecc0e0a..617504aff 100644 --- a/src/cmd/ksh93/sh/parse.c +++ b/src/cmd/ksh93/sh/parse.c @@ -1465,10 +1465,10 @@ static Shnode_t *simple(Lex_t *lexp,int flag, struct ionod *io) { /* check for builtin command */ Namval_t *np=nv_bfsearch(argp->argval,lexp->sh->fun_tree, (Namval_t**)&t->comnamq,(char**)0); - if(cmdarg==0) - t->comnamp = (void*)np; if(np && is_abuiltin(np)) { + if(cmdarg==0) + t->comnamp = (void*)np; if(nv_isattr(np,BLT_DCL)) { assignment = 1+(*argp->argval=='a'); diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index a8ea287fb..04840235a 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -402,7 +402,7 @@ Dt_t *sh_subfuntree(int create) register struct subshell *sp = subshell_data; if(!sp || sp->shp->curenv==0) return(sh.fun_tree); - if(!sp->sfun && create) + if(!sp->sfun && create && !sp->shp->subshare) { sp->sfun = dtopen(&_Nvdisc,Dtoset); dtview(sp->sfun,sp->shp->fun_tree); diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh index c9d3f21bc..9bcede4de 100755 --- a/src/cmd/ksh93/tests/subshell.sh +++ b/src/cmd/ksh93/tests/subshell.sh @@ -623,4 +623,34 @@ do if [[ -e $f ]] fi done +# ====== +# Unsetting or redefining functions within subshells + +# ...function can be unset in subshell + +func() { echo mainfunction; } +(unset -f func; typeset -f func >/dev/null 2>&1) && err_exit 'function fails to be unset in subshell' +v=$(unset -f func; typeset -f func >/dev/null 2>&1) && err_exit 'function fails to be unset in comsub' +v=${ unset -f func 2>&1; } && ! typeset -f func >/dev/null 2>&1 || err_exit 'function unset fails to survive ${ ...; }' + +# ...function can be redefined in subshell +func() { echo mainfunction; } +(func() { echo sub; }; [[ ${ func; } == sub ]]) || err_exit 'function fails to be redefined in subshell' +v=$(func() { echo sub; }; func) && [[ $v == sub ]] || err_exit 'function fails to be redefined in comsub' +v=${ { func() { echo sub; }; } 2>&1; } && [[ $(PATH=/dev/null func) == sub ]] \ + || err_exit 'function redefine fails to survive ${ ...; }' + +# ...some more fun from Stéphane Chazelas: https://github.com/att/ast/issues/73#issuecomment-384178095 +a=$tmp/morefun.sh +cat >| "$a" <