ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
182 stars 31 forks source link

command substitution botches output of non-waited-for child processes #124

Open McDutchie opened 4 years ago

McDutchie commented 4 years ago

Reproducer:

echo "<$(sh -c '(sleep 1; echo after) &'; echo now)>"

Actual output:

<after
now>

Expected output:

<now
after>

Previous report/discussion for ksh2020: https://github.com/att/ast/issues/1479

According to @stephane-chazelas's analysis, this regression was introduced in 93u+ as command substitutions switched from pipes to temporary files. The problems with that (including a potential privacy problem) are detailed in his report. We should consider reimporting the method with pipes.

McDutchie commented 4 years ago

According to @stephane-chazelas's analysis, this regression was introduced in 93u+ as command substitutions switched from pipes to temporary files.

However, I can also reproduce the problem in these ksh versions:

$ /usr/local/bin/ksh_2011-02-08 --version                                                                                               
  version         sh (AT&T Research) 93u 2011-02-08
$ /usr/local/bin/ksh_2011-02-08 -c 'echo "<$(sh -c '\''(sleep 1; echo after) &'\''; echo now)>"'
<after
now>
$ /usr/local/bin/ksh_2010-10-10 --version                                                       
  version         sh (AT&T Research) 93u- 2010-10-10
$ /usr/local/bin/ksh_2010-10-10 -c 'echo "<$(sh -c '\''(sleep 1; echo after) &'\''; echo now)>"'
<after
now>

Version s- is broken in the same way as ksh2020:

$ /usr/local/bin/ksh_1993-12-28 --version
  version         sh (AT&T Research) 1993-12-28 s+
$ /usr/local/bin/ksh_1993-12-28 -c 'echo "<$(sh -c '\''(sleep 1; echo after) &'\''; echo now)>"' 
<now>
McDutchie commented 3 years ago

Note that 970069a6feb71424f4a98b9f3005181eeaa1c448 has now changed the behaviour of this bug as signalled by @JohnoKing here: https://github.com/ksh93/ksh/issues/104#issuecomment-667622512

stephane-chazelas commented 3 years ago

Previous report/discussion for ksh2020: att#1479

According to @stephane-chazelas's analysis, this regression was introduced in 93u+ as command substitutions switched from pipes to temporary files. The problems with that (including a potential privacy problem) are detailed in his report. We should consider reimporting the method with pipes.

No, the main issue I was reporting there is separate from this one and was introduced after ksh93u+ (either v- or later).

McDutchie commented 3 years ago

Interesting – after 970069a, these act differently:

$ echo "<$(sh -c '(sleep 1; echo after) &'; echo now)>"
<now>
$ echo "<`sh -c '(sleep 1; echo after) &'; echo now`>" 
<after
now>

Both still broken, but differently. comsub==1 means the `obsolete style` command substitutions.

JohnoKing commented 3 years ago

While testing this bug I've found that the behavior changes depending on the argument passed to sh_subtmpfile. The below patch fixes the bug for backtick command substitutions, but unfortunately introduces a hang in the regression tests:

diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index a610d270..21ccc071 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1564,7 +1564,7 @@ int sh_exec(register const Shnode_t *t, int flags)
            int pipes[3];
            if(shp->subshell)
            {
-               sh_subtmpfile(2);
+               sh_subtmpfile(shp->comsub);
                if((shp->comsub && (type&(FAMP|TFORK))==(FAMP|TFORK) || shp->comsub==1) &&
                !(shp->fdstatus[1]&IONOSEEK))
                    unpipe = iousepipe(shp);
$ arch/*/bin/ksh -c $'echo "<`sh -c \'(sleep 1; echo after) &\'; echo now`>"; echo "<$(sh -c \'(sleep 1; echo after) &\'; echo now)>"'  
<now
after>
<now>
$ bin/shtests -p subshell
#### Regression-testing /home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh ####
test subshell begins at 2021-03-26+08:42:44
    subshell.sh[819]: backtick comsub hang (got status 265/KILL)
test subshell failed at 2021-03-26+08:42:53 with exit code 1 [ 119 tests 1 error ]
Total errors: 1

Changing the argument given to sh_subtmpfile to '1' will also fix the bug in normal command substitutions, but that introduces even more regression test failures.

McDutchie commented 3 years ago

As of 42becab63cae760addae6e88008665be352bdbbd there is no more execution-level difference between backtick command substitutions and modern ones, as the former now acts like the latter. So the behaviour is now at least consistent between the two:

$ echo "<$(sh -c '(sleep 1; echo after) &'; echo now)>"
<now>
$ echo "<`sh -c '(sleep 1; echo after) &'; echo now`>" 
<now>