Fix <>; redirection for final command exec optimization (#277)

The <>; operator doesn't work correctly if it's used as the last
command of a -c script. Reproducer:
  $ echo test > a; ksh -c 'echo x 1<>; a'; cat a
  x
  st
This bug is caused by ksh running the last command of -c scripts
with execve(2) instead of posix_spawn(3) or fork(2). The <>;
operator is noted by the man page as being incompatible with the
exec builtin (see also the ksh93u+ man page), so it's not
surprising this bug occurs when ksh runs a command using execve:

> <>;word cannot be used with the exec and redirect built-ins.

The ksh2020 fix simply removed the code required for ksh to use
this optimization at all. It's not a performance friendly fix and
only papers over the bug, so this commit provides a better fix.

This bug was first reported at:
https://github.com/att/ast/issues/9

In addition, this commit re-enables the execve(2) optimization for
the last command for scripts loaded from a file. It was enabled in
in older ksh versions, and was only disabled in interactive shells:
https://github.com/ksh93/ast-open-history/blob/2011-06-30/src/cmd/ksh93/sh/main.c#L593-L599
It was changed on 2011-12-24 to only be used for -c scripts:
https://github.com/ksh93/ast-open-history/blob/2011-12-24/src/cmd/ksh93/sh/main.c#L593-L599

We think there is no good reason why scripts loaded from a file
should be optimised less than scripts loaded from a -c argument.
They're both scripts; there's no essential difference between them.
So this commit reverts that change. If there is a bug left in the
optimization after this fix, this revert increases the chance of
exposing it so that it can be fixed.

src/cmd/ksh93/sh/xec.c:
- The IOREWRITE flag is set when handling the <>; operator, so to
  fix this bug, avoid exec'ing the last command if it uses <>;. See
  also commit 17ebfbf6, which fixed another issue related to the
  execve optimization.

src/cmd/ksh93/tests/io.sh:
- Enable a regression test that was failing because of this bug.
- Add the reproducer from https://github.com/att/ast/issues/9 as a
  regression test.

src/cmd/ksh93/sh/main.c:
- Only avoid the non-forking optimization in interactive shells.

src/cmd/ksh93/tests/signal.sh:
- Add an extra comment to avoid the non-forking optimization in the
  regression test for rhbz#1469624.
- If the regression test for rhbz#1469624 fails, show the incorrect
  exit status in the error message.

src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/options.sh:
- This bugfix was causing the options regression test to segfault
  when run under shcomp. The cause is the same as
  <https://github.com/ksh93/ksh/issues/165>, so as a workaround,
  avoid parsing process substitutions with shcomp until that is
  fixed. This workaround should also avoid the other problem
  detailed in <https://github.com/ksh93/ksh/issues/274>.

Resolves: https://github.com/ksh93/ksh/issues/274

NEWS

@@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-04-15:
+
+- Fixed an optimization bug that caused the <>; redirection operator to fail
+  when used in -c scripts.
+
 2021-04-14:
 
 - Path-bound built-ins (such as /opt/ast/bin/cat) can now be executed by

src/cmd/ksh93/include/version.h

@@ -20,7 +20,7 @@
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
 #define SH_RELEASE_SVER	"1.0.0-alpha"	/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2021-04-14"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2021-04-15"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */

src/cmd/ksh93/sh/main.c

@@ -575,7 +575,7 @@ static void	exfile(register Shell_t *shp, register Sfio_t *iop,register int fno)
 		{
 			execflags = sh_state(SH_ERREXIT)|sh_state(SH_INTERACTIVE);
 			/* The last command may not have to fork */
-			if(!sh_isstate(SH_PROFILE) && sh_isoption(SH_CFLAG) &&
+			if(!sh_isstate(SH_PROFILE) && !sh_isstate(SH_INTERACTIVE) &&
 				(fno<0 || !(shp->fdstatus[fno]&(IOTTY|IONOSEEK)))
 				&& !sfreserve(iop,0,0))
 			{

src/cmd/ksh93/sh/xec.c

@@ -1003,8 +1003,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 		shp->exitval=0;
 		shp->lastsig = 0;
 		shp->lastpath = 0;
-		if(shp->exittrap || shp->errtrap)
-			execflg = 0;
+		if(shp->exittrap || shp->errtrap || (t->tre.treio && (t->tre.treio->iofile&IOREWRITE)))
+			execflg = 0; /* don't run the command with execve(2) */
 		switch(type&COMMSK)
 		{
 		    case TCOM:

src/cmd/ksh93/tests/builtins.sh

@@ -986,6 +986,9 @@ EOF
 
 # ======
 # Builtins should handle unrecognized options correctly
+# Note: To workaround https://github.com/ksh93/ksh/issues/165, put the list
+# of builtins in a file, then read from that.
+builtin > "$tmp/builtin-list"
 while IFS= read -r bltin <&3
 do	case $bltin in
 	echo | test | true | false | \[ | : | getconf | */getconf | uname | */uname | catclose | catgets | catopen | Dt* | _Dt* | X* | login | newgrp )
@@ -999,7 +1002,7 @@ do	case $bltin in
 	esac
 	[[ $actual == *"$expect"* ]] || err_exit "$bltin should show usage info on unrecognized options" \
 			"(expected string containing $(printf %q "$expect"), got $(printf %q "$actual"))"
-done 3< <(builtin)
+done 3< "$tmp/builtin-list"
 
 # ======
 # The 'alarm' builtin could make 'read' crash due to IFS table corruption caused by unsafe asynchronous execution.

src/cmd/ksh93/tests/io.sh

@@ -500,12 +500,19 @@ actual=$(cat "$tmp/nums1")
 [[ "$actual" = "$expect" ]] || err_exit "Failed to truncate file in subshell \
 (expected $(printf %q "$expect"), got $(printf %q "$actual"))"
 
-: <<\INACTIVE	# TODO: the >#5 is optimised away by a '-c' optimisation corner case bug
+# The <>; redirection operator didn't work correctly in -c scripts
+# https://github.com/att/ast/issues/9
 "$SHELL" -c '1<>;"$1/nums2" >#5' x "$tmp"
 actual=$(cat "$tmp/nums2")
 [[ "$actual" = "$expect" ]] || err_exit "Failed to truncate file in -c script \
 (expected $(printf %q "$expect"), got $(printf %q "$actual"))"
-INACTIVE
+
+echo test > "$tmp/ast-9-reproducer"
+"$SHELL" -c "echo x 1<>; \"$tmp/ast-9-reproducer\""
+exp=x
+got=$(cat "$tmp/ast-9-reproducer")
+[[ $exp == "$got" ]] || err_exit "<>; redirection operator fails in -c scripts \
+(expected $(printf %q "$exp"), got $(printf %q "$got"))"
 
 # ======
 # Exit behaviour of 'exec', 'command exec', 'redirect' on redirections

src/cmd/ksh93/tests/options.sh

@@ -537,8 +537,12 @@ fi # SHOPT_BRACEPAT
 	(set --default -o posix; [[ -o letoctal ]]) && err_exit "set --default failed to stop posix option from changing others"
 	(set --posix; [[ -o letoctal ]]) || err_exit "set --posix fails to enable letoctal"
 	(set -o posix; [[ -o letoctal ]]) || err_exit "set -o posix fails to enable letoctal"
-	$SHELL --posix < <(echo 'exit 0') || err_exit "ksh fails to handle --posix during startup"
-	$SHELL -o posix < <(echo 'exit 0') || err_exit "ksh fails to handle -o posix during startup"
+
+	# Note: To workaround erratic behavior caused by https://github.com/ksh93/ksh/issues/165,
+	# avoid parsing the process substitutions with shcomp by running
+	# the tests with eval.
+	eval "$SHELL --posix < <(echo 'exit 0')" || err_exit "ksh fails to handle --posix during startup"
+	eval "$SHELL -o posix < <(echo 'exit 0')" || err_exit "ksh fails to handle -o posix during startup"
 fi
 
 # ======

src/cmd/ksh93/tests/signal.sh

@@ -450,6 +450,7 @@ let "$? == 256+9" && err_exit 'exit with status > 256 makes shell kill itself'
 cat >"$tmp/sigtest.sh" <<\EOF
 echo begin
 "$1" -c 'kill -9 "$$"'
+# this extra comment disables an exec optimization
 EOF
 expect=$'^begin\n/.*/sigtest.sh: line 2: [1-9][0-9]*: Killed\n[1-9][0-9]{1,2}$'
 actual=$(LANG=C "$SHELL" -c '"$1" "$2" "$1"; echo "$?"' x "$SHELL" "$tmp/sigtest.sh" 2>&1)
@@ -457,7 +458,8 @@ if	! [[ $actual =~ $expect ]]
 then	[[ $actual == *Killed*Killed* ]] && msg='ksh killed itself' || msg='unexpected output'
 	err_exit "$msg after child process signal (expected match to $(printf %q "$expect"); got $(printf %q "$actual"))"
 fi
-let "${actual##*$'\n'} > 128" || err_exit "child process signal did not cause exit status > 128"
+let "${actual##*$'\n'} > 128" || err_exit "child process signal did not cause exit status > 128" \
+	"(got ${actual##*$'\n'})"
 
 # ======
 # Killing a non-existent job shouldn't cause a segfault. Note that `2> /dev/null` has no effect when