Fix pipefail with (errexit or ERR trap) regression

ksh 93u+ introduced a regression in the combination of the
'set -o pipefail' and 'set -e'/'set -o errexit' options:

$ ksh93 -o errexit -o pipefail -c \
	'(exit 3) | true; echo "still here despite $? status"'
still here despite 3 status

The bug is in how the the huge sh_exec() function in xec.c handles
the 'echeck' flag. Near the end of sh_exec(), this flag triggers a
sh_chktrap() call to check whether to trigger any traps, including
the ERR trap -- and that same function also handles the errexit
option, which is basically the same as 'trap "exit" ERR'.

We can learn more easily how sh_exec() works by inserting debug
warnings in all its 'switch(type&COMMSK)' cases, like:

    case TCOM:
	errormsg(SH_DICT,ERROR_warn(0),"[DEBUG] TCOM");

... and same for all the others. With that done, the output
of a very simple dummy pipeline looks as follows:

$ arch/*/bin/ksh -c 'true | true | true'
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFIL
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFORK
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFORK
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TSETIO
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM

So, it looks like sh_exec() handles this pipeline as follows:

	TFIL
	   |_____TFORK
	   |         |_____TCOM
	   |_____TFORK
	   |         |_____TCOM
	   |_____TSETIO
	             |_____TCOM

Each time a pipeline like command1 | command2 | ... is executed,
sh_exec() is invoked with type TFIL; this then recursively invokes
sh_exec() to handle the individual elements. The last element of
the pipe triggers a sh_exec() run with type TSETIO; since it is run
in the current shell environment, it is effectively treated as a
command with an input redirection. All the previous elements are of
type TFORK instead, because they are executed asynchronously in
separate, forked subshell processes. Finally, the TFORK or TSETIO
code then recursively calls sh_exec() again with type TCOM to
actually execute the commands.

When reading the code, we find that the 'echeck' flag is set as
part of the TSETIO code. This makes sense of why only an error in
the last element of the pipe triggers the errexit/ERR trap action.
So that's the bug: the flag is set in the wrong place.

This can be fixed by setting that flag in the TFIL handling code
instead, as this is what calls everything else and collects all the
exit statuses. So the sh_chktrap() call is now executed after
handling the entire pipeline, at the TFIL recursion level.

This also allows getting rid of the special-casing in the buggy
TSETIO version. The SH_ERREXIT state is restored at the end of each
sh_exec() call, so since we're now doing this at a lower recursion
level, it will already have been restored.

src/cmd/ksh93/sh/xec.c: sh_exec():
- Fix the bug as per the above.

src/cmd/ksh93/tests/options.sh:
- Add tests for errexit and ERR trap combined with pipefail.

src/cmd/ksh93/tests/basic.sh:
- Tweak a couple of tests that reported a trap wasn't triggered
  even if it was actually triggered more than once.

Fixes: https://github.com/ksh93/ksh/issues/121
Thanks to Stéphane Chazelas for the bug report.

NEWS

@@ -8,6 +8,12 @@ Any uppercase BUG_* names are modernish shell bug IDs.
 - Fixed: 'typeset -xu' and 'typeset -xl' (export + change case) failed to
   change the case of a variable's value in certain conditions.
 
+- A ksh 93u+ regression was fixed in the combination of ERR trap handling and
+  the 'pipefail' option. A pipeline now triggers the ERR trap correctly again
+  if the 'pipefail' option is active and any of the pipeline elements return a
+  nonzero exit status. Similarly, if both the 'errexit' and 'pipefail' options
+  are active, ksh now correctly exits if any pipeline element returns nonzero.
+
 2020-09-28:
 
 - While executing a ksh-style function, ksh 93u+ ignored all signals for which

src/cmd/ksh93/sh/xec.c

@@ -1819,7 +1819,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 				type = shp->exitval;
 				if(!(type&SH_EXITSIG))
 				{
-					/* wait for remainder of pipline */
+					/* wait for remainder of pipeline */
 					if(shp->pipepid>1)
 					{
 						job_wait(shp->pipepid);
@@ -1834,11 +1834,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 					sh_iounpipe(shp);
 				shp->pipepid = 0;
 				shp->st.ioset = 0;
-				if(simple && was_errexit)
-				{
-					echeck = 1;
-					sh_onstate(SH_ERREXIT);
-				}
 			}
 			if(jmpval>SH_JMPIO)
 				siglongjmp(*shp->jmplist,jmpval);
@@ -1908,6 +1903,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 			int	savejobid = job.curjobid;
 			int	*exitval=0,*saveexitval = job.exitval;
 			pid_t	savepgid = job.curpgid;
+			echeck = 1;
 			job.exitval = 0;
 			job.curjobid = 0;
 			if(shp->subshell)

src/cmd/ksh93/tests/basic.sh

@@ -523,9 +523,11 @@ do	print hello
 done |  "$binsleep" .1
 (( (SECONDS-s) < .2 )) || err_exit 'early termination not causing broken pipe'
 
-[[ $({ trap 'print trap' 0; print -n | $(whence -p cat); } & wait $!) == trap ]] || err_exit 'trap on exit not getting triggered'
-var=$({ trap 'print trap' ERR; print -n | $binfalse; } & wait $!)
-[[ $var == trap ]] || err_exit 'trap on ERR not getting triggered'
+got=$({ trap 'print trap' 0; print -n | "$bincat"; } & wait "$!")
+[[ $got == trap ]] || err_exit "trap on exit not correctly triggered (expected 'trap', got $(printf %q "$got"))"
+
+got=$({ trap 'print trap' ERR; print -n | "$binfalse"; } & wait "$!")
+[[ $got == trap ]] || err_exit "trap on ERR not correctly triggered (expected 'trap', got $(printf %q "$got"))"
 
 exp=
 got=$(

src/cmd/ksh93/tests/options.sh

@@ -561,5 +561,15 @@ actual=$(set +B; echo ${ echo test{1,2}; })
 [[ $actual == "$expect" ]] || err_exit 'Brace expansion not turned off in ${ comsub; }' \
 	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
 
+# ======
+# ksh 93u+ did not correctly handle the combination of pipefail and (errexit or the ERR trap).
+# https://github.com/ksh93/ksh/issues/121
+got=$("$SHELL" -o errexit -o pipefail -c '(exit 3) | true; echo "still here despite $? status"' 2>&1)
+let "(e=$?)==3" && [[ -z $got ]] || err_exit 'errexit + pipefail failed' \
+	"(expected status 3, ''; got status $e, $(printf %q "$got"))"
+got=$("$SHELL" -o pipefail -c 'trap "print ERR\ trap" ERR; true | exit 3 | false | true | true' 2>&1)
+let "(e=$?)==1" && [[ $got == 'ERR trap' ]] || err_exit 'ERR trap + pipefail failed' \
+	"(expected status 1, 'ERR trap'; got status $e, $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))