Fix race conditions running external commands with job control on

McDutchie · McDutchie · commit f207cd57879e · 2020-07-22T13:45:33.000+01:00
When ksh is compiled with SHOPT_SPAWN (the default), which uses posix_spawn(3) or vfork(2) (via sh_ntfork()) to launch external commands, at least two race conditions occur when launching external commands while job control is active. See: https://bugs.launchpad.net/ubuntu/+source/ksh/+bug/1887863/comments/3 https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html The basic issue is that this performance optimisation is incompatible with job control, because it uses a spawning mechanism that doesn't copy the parent process' memory pages into the child process, therefore no state that involves memory can be set before exec-ing the external program. This makes it impossible to correctly set the terminal's process group ID in the child process, something that is essential for job control to work. src/cmd/ksh93/sh/xec.c: - Use sh_fork() instead of sh_ntfork() if job control is active. This uses fork(2), which is 30%-ish slower on most sytems, but allows for correctly setting the terminal process group. src/cmd/ksh93/tests/basic.sh: - Add regression test for the race condition reported in #79. src/cmd/INIT/cc.darwin: - Remove hardcoded flag to disable SHOPT_SPAWN on the Mac. It should be safe to use now. Fixes #79
diff --git a/NEWS b/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.
 
+2020-07-22:
+
+- Fixed two race conditions when running external commands on
+  interactive shells with job control active.
+
 2020-07-20:
 
 - If a shell function and a built-in command by the same name exist,
diff --git a/src/cmd/INIT/cc.darwin b/src/cmd/INIT/cc.darwin
@@ -43,7 +43,7 @@ init)	echo "cc: arguments expected" >&2
 	;;
 cpp)	$CC -E "$@"
 	;;
-cc)	$CC -DSHOPT_SPAWN=0 -D_ast_int8_t=int64_t -D_lib_memccpy \
+cc)	$CC -D_ast_int8_t=int64_t -D_lib_memccpy \
 		-Wno-unused-value -Wno-parentheses -Wno-macro-redefined "$@"
 	;;
 dll)	$CC -Wl,-flat_namespace -dynamiclib -undefined dynamic_lookup "$@"
diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h
@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-07-20"
+#define SH_RELEASE	"93u+m 2020-07-22"
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
@@ -1575,7 +1575,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 				}
 #if SHOPT_SPAWN
 #   ifdef _lib_fork
-				if(com)
+				if(com && !job.jobcontrol)
 					parent = sh_ntfork(shp,t,com,&jobid,ntflag);
 				else
 					parent = sh_fork(shp,type,&jobid);
@@ -3460,6 +3460,7 @@ static void sigreset(Shell_t *shp,int mode)
 
 /*
  * A combined fork/exec for systems with slow or non-existent fork()
+ * Note: Incompatible with job control.
  */
 static pid_t sh_ntfork(Shell_t *shp,const Shnode_t *t,char *argv[],int *jobid,int flag)
 {
diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh
@@ -598,5 +598,22 @@ eu=$(
 )
 [[ ${us:1:1} == ${eu:1:1} ]] && err_exit "The time keyword ignores the locale's radix point (both are ${eu:1:1})"
 
+# ======
+# Test for bug in ksh binaries that use posix_spawn() while job control is active.
+# See discussion at: https://github.com/ksh93/ksh/issues/79
+if	test -t 1 2>/dev/null 1>/dev/tty	# this test only works if we have a tty
+then	actual=$(
+		"$SHELL" -i <<-\EOF 2>/dev/tty
+		printf '%s\n' 1 2 3 4 5 | while read
+		do	ls /dev/null
+		done 2>&1
+		exit  # suppress extra newline
+		EOF
+	)
+	expect=$'/dev/null\n/dev/null\n/dev/null\n/dev/null\n/dev/null'
+	[[ $actual == "$expect" ]] || err_exit 'Race condition while launching external commands' \
+		"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
+fi
+
 # ======
 exit $((Errors<125?Errors:125))
