`unset -f whence` removes `whence` builtin in comsub?

[McDutchie]

$ unset -f whence; whence whence  # ok
whence
$ (unset -f whence; whence whence)   # ok
whence
$ got=$(unset -f whence; whence whence) && echo "$got"  # BUG
-ksh: whence: not found
$ got=$(ulimit -c 0; unset -f whence; whence whence) && echo "$got"  # ok
whence

So, the umpteenth weird issue with non-forking subshells.

Testing my stash of compiled ksh commit suggests that this has been introduced or exposed in 13c57e4b58fe38e3d5abb8219e79a1b13b78476e.

[McDutchie]

The problem was indeed introduced in that commit and is entirely my fault.

Patch:

diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 2526a4da8..2866e80bf 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1417,7 +1417,7 @@ static int unall(int argc, char **argv, Dt_t *troot)
        }
        else if(troot==sh.alias_tree)
            r = 1;
-       else if(troot==sh.fun_tree && troot!=sh.fun_base && nv_search(name,sh.fun_tree,0))
+       else if(troot==sh.fun_tree && troot!=sh.fun_base && (np=nv_search(name,sh.fun_tree,0)) && is_afunction(np))
            nv_open(name,troot,NV_NOSCOPE); /* create dummy virtual subshell node without NV_FUNCTION attribute */
    }
    return r;

Root cause of the problem: just because nv_search in the function tree succeeds doesn't mean we can assume what is found is a function, because fun_tree is initialised with a view to bltin_tree, so searching fun_tree finds both functions and builtins. So we need the extra check that it's actually a function.

But I have no idea why this bug is only exposed for whence.

[McDutchie]

$ (unset -f whence; whence whence)   # ok
whence

Of course this succeeds on an interactive shell; I forgot I made all non-comsub subshells of interactive shells fork in d11d4c7acd44ae478fd182a4fa5db9e45f989a04. When this is run from a script (or as a nested subshell in another subshell), it fails as expected.

[McDutchie]

But why on earth does this bug only manifest for whence!?

$ for b in $(builtin|grep '^[[:alpha:]]*$'); do got=$(unset -f $b; whence $b) && echo OK: $b==$got; done
OK: alias==alias
OK: autoload==autoload
OK: bg==bg
OK: break==break
OK: builtin==builtin
OK: cd==cd
OK: command==command
OK: compound==compound
OK: continue==continue
OK: disown==disown
OK: echo==echo
OK: enum==enum
OK: eval==eval
OK: exec==exec
OK: exit==exit
OK: export==export
OK: false==false
OK: fc==fc
OK: fg==fg
OK: float==float
OK: functions==functions
OK: getopts==getopts
OK: hash==hash
OK: hist==hist
OK: integer==integer
OK: jobs==jobs
OK: kill==kill
OK: let==let
OK: nameref==nameref
OK: print==print
OK: printf==printf
OK: pwd==pwd
OK: read==read
OK: readonly==readonly
OK: redirect==redirect
OK: return==return
OK: set==set
OK: shift==shift
OK: sleep==sleep
OK: source==source
OK: stop==stop
OK: suspend==suspend
OK: test==test
OK: times==times
OK: trap==trap
OK: true==true
OK: type==type
OK: typeset==typeset
OK: ulimit==ulimit
OK: umask==umask
OK: unalias==unalias
OK: unset==unset
OK: wait==wait
-ksh: whence: not found

[McDutchie]

But why on earth does this bug only manifest for whence!?

It's because the reproducer is broken. The bug causes a dummy function subshell node to be created, masking the builtin, but that only breaks things if you actually try to run the builtin – whereas the above reproducer only runs whence.

[McDutchie]

The fix in the patch is not good. After the patch the bug still occurs if a function exists in the parent shell:

$ whence() { echo MAIN; }
$ got=$(unset -f whence; whence --version)
arch/darwin.arm64-64/bin/ksh: whence: not found

[McDutchie]

Unless I'm still missing something, this patch should fix it properly. The previous patch above was not wrong, but incomplete. Functions and builtins are looked up via nv_bfsearch() -- one nv_search() call for both, as fun_tree is initialised with a view to bltin_tree. That function needs to be amended to explicitly ignore dummy null nodes in subshell function trees.

diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 2526a4da8..59294283f 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1402,7 +1402,7 @@ static int unall(int argc, char **argv, Dt_t *troot)
            else if(isfun)
            {
                if(troot!=sh.fun_base)
-                   nv_offattr(np,NV_FUNCTION); /* invalidate */
+                   np->nvflag = 0; /* invalidate */
                else if(!(np->nvalue.rp && np->nvalue.rp->running))
                    nv_delete(np,troot,0);
            }
@@ -1417,8 +1417,8 @@ static int unall(int argc, char **argv, Dt_t *troot)
        }
        else if(troot==sh.alias_tree)
            r = 1;
-       else if(troot==sh.fun_tree && troot!=sh.fun_base && nv_search(name,sh.fun_tree,0))
-           nv_open(name,troot,NV_NOSCOPE); /* create dummy virtual subshell node without NV_FUNCTION attribute */
+       else if(troot==sh.fun_tree && troot!=sh.fun_base && (np=nv_search(name,sh.fun_tree,0)) && is_afunction(np))
+           nv_open(name,troot,NV_NOSCOPE); /* create null virtual subshell node to mask parent tree function */
    }
    return r;
 }
diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index fcb824ab2..0ada2f342 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -1061,7 +1061,17 @@ Namval_t *nv_bfsearch(const char *name, Dt_t *root, Namval_t **var, char **last)
            cp = sp; 
    }
    if(!cp)
-       return var ? nv_search(name,root,0) : 0;
+   {
+       if(!var)
+           return 0;
+       np = nv_search(name,root,0);
+       /* If 'unset -f' set a dummy null node in a subshell function tree, then
+        * we must ignore this, but without searching in the parent function tree;
+        * search again, directly in the builtins tree. */
+       if(np && !np->nvflag && !np->nvalue.rp && root==sh.fun_tree && root!=sh.fun_base)
+           np = nv_search(name,sh.bltin_tree,0);
+       return np;
+   }
    sfputr(sh.stk,name,0);
    dname = cp+1;
    cp = stkptr(sh.stk,offset) + (cp-name); 

[McDutchie]

Still not fully fixed. :-(

$ arch/*/bin/ksh -c 'whence() { echo main; }; (unset -f whence; PATH=/dev/null; whence --version); whence --version'    
arch/darwin.arm64-64/bin/ksh: whence: not found
main

but:

$ arch/*/bin/ksh -c 'whence() { echo main; }; got=$(unset -f whence; PATH=/dev/null; whence --version); whence --version'
  version         whence (ksh 93u+m) 2023-05-30
main

[McDutchie]

Patch to, hopefully, fix the rest.

The important change is in xec.c: we need to call nv_bfsearch() not only if the node doesn't exist, but also if the node is null (i.e. has no nvflag attributes). The fix already applied to nv_bfsearch() then does the rest.

sh_fsearch() also needs to handle null nodes for namespace contexts.

diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index 0ada2f342..52026f1d9 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -1435,12 +1435,20 @@ int nv_hasget(Namval_t *np)
 }

 #if SHOPT_NAMESPACE
-Namval_t *sh_fsearch(const char *fname, int add)
+Namval_t *sh_fsearch(const char *fname, int mode)
 {
    int offset = stktell(sh.stk);
    sfputr(sh.stk,nv_name(sh.namespace),'.');
    sfputr(sh.stk,fname,0);
    fname = stkptr(sh.stk,offset);
-   return nv_search(fname,sh_subfuntree(add&NV_ADD),add);
+   if(!(mode&NV_ADD))
+   {
+       Namval_t *np = nv_search(fname,sh.fun_tree,mode);
+       /* do not return a dummy subshell node set by 'unset -f' */
+       if(np && !np->nvflag)
+           return NULL;
+       return np;
+   }
+   return nv_search(fname,sh_subfuntree(mode&NV_ADD),mode);
 }
 #endif /* SHOPT_NAMESPACE */
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 77a44a189..56ff23732 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -971,7 +971,7 @@ int sh_exec(const Shnode_t *t, int flags)
            }
            if(com0)
            {
-               if(!np && !strchr(com0,'/'))
+               if((!np || !np->nvflag) && !strchr(com0,'/'))
                {
                    Dt_t *root = command?sh.bltin_tree:sh.fun_tree;
                    np = nv_bfsearch(com0, root, &nq, &cp);

[McDutchie]

New approach to the patch, works at least as well as the last one: instead of fixing sh_fsearch() and nv_bfsearch(), fix the lower-level function nv_search() that they both use. This is more robust and future-proof, and could possibly fix other undiscovered issues.

diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index 0ada2f342..10d8f575b 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -1007,6 +1007,9 @@ Namval_t *nv_search(const char *name, Dt_t *root, int mode)
            root = sh.var_base;
        np = dtmatch(root,name);
    }
+   /* skip dummy shell function node (function unset in virtual subshell) */
+   if(np && !np->nvflag && root==sh.fun_tree)
+       np = mode&NV_NOSCOPE ? NULL : dtmatch(sh.bltin_tree,name);
    if(!np && (mode&NV_ADD))
    {
        if(sh.namespace && !(mode&NV_NOSCOPE) && root==sh.var_tree)
@@ -1017,7 +1020,7 @@ Namval_t *nv_search(const char *name, Dt_t *root, int mode)
            while(next=dtvnext(root))
                root = next;
        }
-       np = (Namval_t*)dtinsert(root,newnode(name));
+       np = (Namval_t*)dtinstall(root,newnode(name));
    }
    if(dp)
        dtview(root,dp);
@@ -1061,17 +1064,7 @@ Namval_t *nv_bfsearch(const char *name, Dt_t *root, Namval_t **var, char **last)
            cp = sp; 
    }
    if(!cp)
-   {
-       if(!var)
-           return 0;
-       np = nv_search(name,root,0);
-       /* If 'unset -f' set a dummy null node in a subshell function tree, then
-        * we must ignore this, but without searching in the parent function tree;
-        * search again, directly in the builtins tree. */
-       if(np && !np->nvflag && !np->nvalue.rp && root==sh.fun_tree && root!=sh.fun_base)
-           np = nv_search(name,sh.bltin_tree,0);
-       return np;
-   }
+       return var ? nv_search(name,root,0) : NULL;
    sfputr(sh.stk,name,0);
    dname = cp+1;
    cp = stkptr(sh.stk,offset) + (cp-name); 
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 77a44a189..56ff23732 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -971,7 +971,7 @@ int sh_exec(const Shnode_t *t, int flags)
            }
            if(com0)
            {
-               if(!np && !strchr(com0,'/'))
+               if((!np || !np->nvflag) && !strchr(com0,'/'))
                {
                    Dt_t *root = command?sh.bltin_tree:sh.fun_tree;
                    np = nv_bfsearch(com0, root, &nq, &cp);