ksh93: Changes to functions within subshells are ignored

[McDutchie]

Function definitions within non-forked subshells (including command substitutions) are silently ignored if a function by the same name exists in the main shell, so the wrong code is executed. unset -f is also silently ignored. Test script:

foo() { echo WRONG; }
(foo() { echo ok; } && foo && unset -f foo && foo)

Output:

WRONG
WRONG

Forcing the subshell to be forked (by making it a background job, or by including a redirection within a command substitution) is an effective workaround.

Silently executing the wrong code under any circumstances is a bad bug, but it seems to have been in ksh93 since 1993...

[krader1961]

This is definitely a bug. But the subject line is misleading. The problem is that unless you force the (...) list of commands to be executed in a subshell you get the wrong behavior. The (...) notation does not force the creation of a subshell. It only creates a "separate environment". Whatever that means as the term appears to be unused anywhere else in the documentation or source code. I too used to think it created a subshell but it doesn't. Try this: echo $$; ( echo $$ ); ( echo $$ ) & to see what I mean.

Some things, most notably variable assignment, are handled as expected by parenthesized blocks:

$ x=1; echo $x $$; (x=2; echo $x $$); echo $x $$
1 51108
2 51108
1 51108

[McDutchie]

No, your understanding is incorrect. The (...) notation always creates a subshell, but ksh93 by default implements subshells without forking a new process.

Your example echo $$; ( echo $$ ); ( echo $$ ) & simply prints the PID of the main shell three times. That's because $$ doesn't change (on any shell) when a subshell is created (whether by forking or not) because by definition it represents the PID of the main shell.

Instead, try this:

$ echo ${.sh.subshell}; (echo ${.sh.subshell}; (echo ${.sh.subshell}); echo ${.sh.subshell})
0
1
2
1

The subshell counter goes up as the subshell level increases.

[stephane-chazelas]

Some more fun:

$ cat a
foo() { echo WRONG; }
(foo() { echo ok; } && foo && unset -f foo && foo)
$ ksh ./a
WRONG
WRONG
$ ksh -c "$(cat a)"
ok
ksh: line 2: foo: not found
$ ksh -c 'eval "$(cat a)"'
ok
ksh: eval: line 2: foo: not found
$ ksh -c '. ./a'
ok
WRONG

[McDutchie]

@stephane-chazelas gave an essential hint in a comment on #480: running the ulimit builtin in a subshell forces it to fork off into a separate process right then and there.

So I went to look in the source code for how the ulimit builtin does that. And it's surprisingly simple. It's this little line in bltins/ulimit.c:

if (shp->subshell && !shp->subshare) sh_subfork();

With a lot of grepping, I figured out that shp->subshell refers to subshells of the non-forking variety (recall that ${.sh.subshell} gets reset to zero when a subshell is forked!) and shp->subshare refers to command substitutions of the ${ ...; } variety, which are actually considered to be subshells that share their state with the parent shell (see the comments on the definitions of SH_SUBSHARE and char subshare in include/shell.h).

So the line above means: if we're in a subshell of the non-forking variety, and it's not the ${ ...; } variant, then fork off and make the forked process continue where we left off (that is what sh_subfork() does).

And forking avoids #73. Thus, I am fairly confident that the following patch fixes it, without messing with complicated code. It seems to fix the oddities observed by Stéphane in the comment above as well.

diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 64f38b7..7643de4 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1127,6 +1127,10 @@ static int unall(int argc, char **argv, Dt_t *troot, Shell_t *shp) {
     while (r = optget(argv, name)) {
	 switch (r) {
	     case 'f': {
+                if (shp->subshell && !shp->subshare) {
+                    // When unsetting a function in a subshell, fork to avoid https://github.com/att/ast/issues/73
+                    sh_subfork();
+                }
		 troot = sh_subfuntree(shp, 1);
		 break;
	     }
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 9d674bc..b39cfd5 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -2350,6 +2350,10 @@ int sh_exec(Shell_t *shp, const Shnode_t *t, int flags) {
		 char *fname = ((struct functnod *)t)->functnam;
		 Namval_t *npv = 0, *mp;
		 cp = strrchr(fname, '.');
+                if (shp->subshell && !shp->subshare) {
+                    // When (re)defining a function in a subshell, fork to avoid https://github.com/att/ast/issues/73
+                    sh_subfork();
+                }
 #if SHOPT_COSHELL
		 if (shp->inpool) {
		     sh_exec(shp, t->funct.functtre, 0);

[kernigh]

This looks broken to me:

foo() { echo WRONG; }
(foo() { echo ok; } && foo)

outputs WRONG, but

foo() { echo WRONG; }; (foo() { echo ok; } && foo)

outputs ok. The only change is from a newline to a semicolon. There is a similar problem with aliases, as I just mentioned in #471.

[kernigh]

I applied the patch by @McDutchie (using patch -l -p1 because of a whitespace problem). On my machine running OpenBSD/amd64 6.3, this patch causes failures in at least meson test coprocess and meson test namespace.

The patch uses sh_subfork() to fork the subshell so that sh_subfuntree() doesn't create the subshell's function tree. The patch doesn't fix autoload:

$ cat fee
fee() { echo loaded; }
$ cat try.sh
foo() { echo WRONG; }
FPATH=.
(autoload fee && foo() { echo ok; } && foo)
$ nksh try.sh
WRONG

This is because autoload fee called sh_subfuntree() and created the tree. The work around is to call sh_subfork() from sh_subfuntree(). Undo that patch and apply this one (but coprocess and namespace still fail):

diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index 90196bf2..97b8e3eb 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -373,10 +373,15 @@ Dt_t *sh_subfuntree(Shell_t *shp, int create) {
     struct subshell *sp = subshell_data;
     if (!sp || shp->curenv == 0) return (shp->fun_tree);
     if (!sp->sfun && create) {
+#if 1
+        // Fork to avoid https://github.com/att/ast/issues/73
+        sh_subfork();
+#else
         sp->sfun = dtopen(&_Nvdisc, Dtoset);
         dtuserdata(sp->sfun, shp, 1);
         dtview(sp->sfun, shp->fun_tree);
         shp->fun_tree = sp->sfun;
+#endif
     }
     return sp->shp->fun_tree;
 }

---

I also have a different patch that fixes function replacement, but doesn't fix unset -f. Undo the other patches and apply this one:

diff --git a/src/cmd/ksh93/sh/parse.c b/src/cmd/ksh93/sh/parse.c
index d8020382..9a6f642c 100644
--- a/src/cmd/ksh93/sh/parse.c
+++ b/src/cmd/ksh93/sh/parse.c
@@ -1370,8 +1370,8 @@ static Shnode_t *simple(Lex_t *lexp, int flag, struct ionod *io) {
                     np = nv_bfsearch(argp->argval, lexp->sh->fun_tree, (Namval_t **)&t->comnamq,
                                      NULL);
                 }
-                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 + !strcmp(argp->argval, "alias");
                         if (np == SYSTYPESET) {

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. The patch 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.

With this patch, unset -f is still wrong:

$ cat fee    
fee() { echo loaded; }
$ cat try.sh
foo() { echo WRONG; }
FPATH=.
(autoload fee && foo() { echo ok; } && foo)
(unset -f foo && foo)
$ nksh try.sh
ok
WRONG

[saper]

I don't think it is a bug, although it feels strange: ( introduces not only a subshell (@krader1961 a subshell is a "separate environment" according to the Korn Shell book) but also is a compound command.

If the first token of the command is one of the following operators:
( ksh reads it as a compound command, until the matching closing ).

Also semicolon separated commands are not equivalent to newline separated commands.

I am still wrapping my head around this but it makes sense when reading the book and is consistent.

 a; b

is a list command, which is a compound command - which is read all at once.

It is not equivalent to

a
b

which are two simple commands.

[cassioneri]

The following seems another manifestation of this bug which, through an extra level of indirection, makes assignments in subshells to affect unset variables in the parent shell:

$ cat bug.sh
#!/bin/ksh93

unset a b
c=0

function set_ac { a=1; c=1; }
function set_abc { ( set_ac ; b=1 ) }

set_abc
echo "a=$a b=$b c=$c"
$ ./bug.sh
a=1 b= c=0

As mentioned by @McDutchie in #73 (comment) and #478 (comment) the following are known workarounds. Replace the definition of set_abc with either

function set_abc { ( set_ac ; b=1 ) | echo -n; }

or

function set_abc { ( ulimit -t unlimited ; set_ac ; b=1 ) }

Here is a third workaround that shows how bad subshells step on each other's toes. By setting a in a second subshell, it prevents the first subshell to set a in the parent shell.

function set_abc { ( set_ac ; b=1 ) }
( a=1 )

With either of the three above we get

$ ./bug.sh
a= b= c=0

[McDutchie]

Hi @cassioneri , thank you for that report. This AT&T repo is no longer active, but the (to the best of my knowledge) only actively and openly developed fork of ksh93 is at https://github.com/ksh93/ksh -- with much of this bug already fixed, but sure enough, your test script exposes another bug that's not fixed yet. Would you mind opening an issue there, please?

[cassioneri]

@McDutchie. Will do.

[McDutchie]

Now that I'm here, I'd like to give belated thanks to @kernigh for the parser fix and to @stephane-chazelas for the reproducers, both of which I've incorporated in what is, to the best of my knowledge, currently the only actively and openly developed fork of ksh93. It's based off the "stable" 93u+ 2012-08-01 version (which might be the least buggy version ever released but is still nothing like actually stable). See my new repo, particularly 047cb33 and 759157b. I'd like to invite you to follow the development and provide any input you feel called to give. The idea is to get a rock-solid ksh93 release out there, so the more eyes on this code, the better.