From 16b380214892cd13226f8b4d085d2e8e4e37767a Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 18 Jun 2022 11:21:03 +0100 Subject: [PATCH] Fix incorrect exec optimisation with monitor/pipefail on Reproducer script: tempfile=/tmp/out2.$$.$RANDOM bintrue=$(whence -p true) for opt in monitor pipefail do ( set +x -o "$opt" ( sleep .05 echo "ok $opt" >&2 ) 2>$tempfile | "$bintrue" ) & wait cat "$tempfile" rm -f "$tempfile" done Expected output: ok monitor ok pipefail Actual output: (none) The 'monitor' and 'pipefail' options are supposed to make the shell wait for the all commands in the pipeline to terminate and not only the last component, regardless of whether the pipe between the component commands is still open. In the failing reproducer, the dummy external true command is subject to an exec optimization, so it replaces the subshell instead of forking a new process. This is incorrect, as the shell is no longer around to wait for the left-hand part of the pipeline, so it continues in the background without being waited for. Since it writes to standard error after .05 seconds (after the pipe is closed), the 'cat' command reliably finds the temp file empty. Without the sleep this would be a race condition with unpredictable results. Interestingly, this bug is only triggered for a (background subshell)& and not for a forked (regular subshell). Which means the exec optimization is not done for a forked regular subshell, though there is no reason not to. That will be fixed in the next commit. src/cmd/ksh93/sh/xec.c: sh_exec(): - case TFORK: Never allow an exec optimization if we're running a command in a multi-command pipeline (pipejob is set) and the shell needs to wait for all pipeline commands, i.e.: either the time keyword is in use, the SH_MONITOR state is active, or the SH_PIPEFAIL option is on. - case TFIL: Fix the logic for setting job.waitall for the non-SH_PIPEFAIL case. Do not 'or' in the boolean value but assign it, and include the SH_TIMING (time keyword in use) state too. - case TTIME: After that fix in case TFIL, we don't need to bother setting job.waitall explicitly here. src/cmd/ksh93/sh.1: - Add missing documentation for the conditions where the shell waits for all pipeline components (time, -o monitor/pipefail). Resolves: https://github.com/ksh93/ksh/issues/449 --- NEWS | 7 +++++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh.1 | 28 ++++++++++++++++++++-------- src/cmd/ksh93/sh/xec.c | 13 ++++++++----- src/cmd/ksh93/tests/options.sh | 20 ++++++++++++++++++++ 5 files changed, 56 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 3d956a91a..04c8df8f9 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,13 @@ For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0 Any uppercase BUG_* names are modernish shell bug IDs. +2022-06-18: + +- Fixed a bug where, with the monitor or pipefail option on, the shell + failed to wait for all component commands in a pipeline to terminate if + the last component command was an external command and the pipeline was + the last command in a background subshell. + 2022-06-15: - Fixed a bug where converting an indexed array into an associative array in diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 92cafd923..81eebcff8 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -23,7 +23,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-06-15" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2022-06-18" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh.1 b/src/cmd/ksh93/sh.1 index ee7df4b57..07ec05408 100644 --- a/src/cmd/ksh93/sh.1 +++ b/src/cmd/ksh93/sh.1 @@ -179,7 +179,7 @@ built-in utility). A .I pipeline\^ is a sequence of one or more -.I commands\^ +.IR command\^ s separated by .BR | . The standard output of each command but the last @@ -194,10 +194,19 @@ by a to the standard input of the next command. Each command, except possibly the last, -is run as a separate process; -the shell waits for the last command to terminate. +is run as a separate process. +If the +.B monitor +or +.B pipefail +option is on, +or the pipeline is preceded by the +.I "reserved word" +.BR time , +then the shell waits for all component commands in the pipeline to terminate; +otherwise, the shell only waits for the last component command. The exit status of a pipeline is the exit -status of the last command unless the +status of its last component command, unless the .B pipefail option is enabled. Each pipeline can be preceded by the @@ -7524,6 +7533,8 @@ not just those that precede the command name. Background jobs will run in a separate process group and a line will print upon completion. The exit status of background jobs is reported in a completion message. +A pipeline will not terminate until all component commands of the pipeline +have terminated. On systems with job control, this option is turned on automatically for interactive shells. @@ -7684,10 +7695,11 @@ Same as .BR \-u . .TP 8 .B pipefail -A pipeline will not complete until all components -of the pipeline have completed, and the return value -will be the value of the last non-zero command -to fail or zero if no command has failed. +The exit status of the entire pipeline will be that of the last component +command that exited with a non-zero exit status, or zero if no command +exited with a non-zero exit status. +The shell will wait for all component commands of the pipeline to terminate, +instead of only waiting for the last component command. .TP 8 .B posix Enables the POSIX standard mode for maximum compatibility with other compliant diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 9d362241a..65c3c805c 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1506,7 +1506,8 @@ int sh_exec(register const Shnode_t *t, int flags) && !sh.st.trapcom[0] && !sh.st.trap[SH_ERRTRAP] && ((struct checkpt*)sh.jmplist)->mode!=SH_JMPEVAL - && (execflg2 || (execflg && sh.fn_depth==0 && !(pipejob && sh_isoption(SH_PIPEFAIL)))); + && (execflg && sh.fn_depth==0 || execflg2) + && !(pipejob && (sh_isstate(SH_MONITOR) || sh_isoption(SH_PIPEFAIL) || sh_isstate(SH_TIMING))); if(no_fork) job.parent=parent=0; else @@ -1892,6 +1893,10 @@ int sh_exec(register const Shnode_t *t, int flags) sh.inpipe = pvo; sh.outpipe = pvn; pvo[1] = -1; + /* + * If the pipefail or monitor options are on or if the time keyword is in use, then wait + * for all commands in the pipeline to complete; otherwise, wait for the last one only + */ if(sh_isoption(SH_PIPEFAIL)) { const Shnode_t* tn=t; @@ -1903,7 +1908,7 @@ int sh_exec(register const Shnode_t *t, int flags) memset(exitval,0,job.waitall*sizeof(int)); } else - job.waitall |= (!pipejob && sh_isstate(SH_MONITOR)); + job.waitall = !pipejob && (sh_isstate(SH_MONITOR) || sh_isstate(SH_TIMING)); job_lock(); nlock++; do @@ -2323,7 +2328,7 @@ int sh_exec(register const Shnode_t *t, int flags) } if(t->par.partre) { - long timer_on = sh_isstate(SH_TIMING); + int timer_on = sh_isstate(SH_TIMING); #ifdef timeofday /* must be run after forking a subshell */ timeofday(&tb); @@ -2336,12 +2341,10 @@ int sh_exec(register const Shnode_t *t, int flags) UNREACHABLE(); } #endif - job.waitall = 1; sh_onstate(SH_TIMING); sh_exec(t->par.partre,sh_isstate(SH_ERREXIT)|OPTIMIZE); if(!timer_on) sh_offstate(SH_TIMING); - job.waitall = 0; } else { diff --git a/src/cmd/ksh93/tests/options.sh b/src/cmd/ksh93/tests/options.sh index 05097f59c..66d22125a 100755 --- a/src/cmd/ksh93/tests/options.sh +++ b/src/cmd/ksh93/tests/options.sh @@ -605,5 +605,25 @@ EOF exit EOF +# ====== +# https://github.com/ksh93/ksh/issues/449 +set +o monitor +o pipefail +for opt in monitor pipefail +do + outfile=out$opt + exp="ok $opt" + ( + set +x -o "$opt" + ( + sleep .01 + print "$exp" >&2 + ) 2>$outfile | "${ whence -p true; }" # the external 'true' should not be exec-optimized + ) & + wait "$!" + got=$(<$outfile) + [[ $got == "$exp" ]] || err_exit "shell did not wait for entire pipeline with -o $opt" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" +done + # ====== exit $((Errors<125?Errors:125))