Fix crash in backtick comsubs with job control on (rhbz#825520)

McDutchie · McDutchie · commit ce68e1be37c3 · 2020-09-23T01:56:09.000+02:00
This imports another fix from Red Hat/Fedora. Original patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-crash.patch src/cmd/ksh93/include/jobs.h, src/cmd/ksh93/sh/jobs.c, src/cmd/ksh93/sh/subshell.c, src/cmd/ksh93/sh/xec.c: - Import the Red Hat fix with these differences: - Rename the 'hack1_waitall' variable to 'bktick_waitall' and add a comment describing what it's for. - Remove unused 'pipefail' variable. src/cmd/ksh93/tests/basic.sh: - Regression test from reproducer given in the Red Hat bug report. - Add special handling to SIGKILL it, as it might freeze hard.
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-09-23:
+
+- Fixed a crash that could occur when running a pipeline containing
+  backtick-style command substitutions with job control enabled.
+
 2020-09-21:
 
 - A bug was fixed that caused command substitutions embedded in here-documents
diff --git a/src/cmd/ksh93/include/jobs.h b/src/cmd/ksh93/include/jobs.h
@@ -99,6 +99,7 @@ struct jobs
 	char		jobcontrol;	/* turned on for real job control */
 	char		waitsafe;	/* wait will not block */
 	char		waitall;	/* wait for all jobs in pipe */
+	char		bktick_waitall;	/* wait state for `backtick comsubs` */
 	char		toclear;	/* job table needs clearing */
 	unsigned char	*freejobs;	/* free jobs numbers */
 };
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-09-21"
+#define SH_RELEASE	"93u+m 2020-09-23"
diff --git a/src/cmd/ksh93/sh/jobs.c b/src/cmd/ksh93/sh/jobs.c
@@ -1803,6 +1803,7 @@ static int job_chksave(register pid_t pid)
 	{
 		count = bp->count;
 		jp = bp->list;
+		jpold = 0;
 		goto again;
 	}
 	if(jp)
diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
@@ -524,7 +524,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 	sp->comsub = shp->comsub;
 	shp->subshare = comsub==2 ||  (comsub==1 && sh_isoption(SH_SUBSHARE));
 	if(comsub)
+	{
 		shp->comsub = comsub;
+		job.bktick_waitall = (comsub==1);
+	}
 	if(!comsub || !shp->subshare)
 	{
 		struct subshell *xp;
@@ -655,6 +658,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 		}
 		else
 		{
+			job.bktick_waitall = 0;
 			if(comsub!=1 && shp->spid)
 			{
 				job_wait(shp->spid);
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
@@ -1927,7 +1927,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 |= (job.bktick_waitall || !pipejob && sh_isstate(SH_MONITOR));
 			job_lock();
 			nlock++;
 			do
diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh
@@ -32,6 +32,7 @@ integer Errors=0
 
 bincat=$(whence -p cat)
 binecho=$(whence -p echo)
+binfalse=$(whence -p false)
 # make an external 'sleep' command that supports fractional seconds
 binsleep=$tmp/.sleep.sh  # hide to exclude from simple wildcard expansion
 cat >"$binsleep" <<EOF
@@ -418,7 +419,6 @@ expected=foreback
 got=$(print -n fore; (sleep .2;print back)&)
 [[ $got == $expected ]] || err_exit "command substitution background process output error -- got '$got', expected '$expected'"
 
-binfalse=$(whence -p false)
 for false in false $binfalse
 do	x=$($false) && err_exit "x=\$($false) should fail"
 	$($false) && err_exit "\$($false) should fail"
@@ -692,5 +692,23 @@ actual=$(exptest foo)
 [[ $actual == "$expect" ]] || err_exit 'Corruption of multibyte char following expansion of single-char name' \
 	"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
 
+# ======
+# Crash in job handling code when running backtick-style command substitutions (rhbz#825520)
+# The regression sometimes doesn't just crash, but freezes hard, so requires special handling.
+cat >$tmp/backtick_crash.ksh <<'EOF'
+binfalse=$(whence -p false) || exit
+for ((i=0; i<250; i++))
+do	test -z `"$binfalse" | "$binfalse" | /dev/null/nothing`
+done
+EOF
+"$SHELL" -i "$tmp/backtick_crash.ksh" 2>/dev/null &	# run test as bg job
+test_pid=$!
+(sleep 10; kill -s KILL "$test_pid" 2>/dev/null) &	# another bg job to kill frozen test job
+sleep_pid=$!
+{ wait "$test_pid"; } 2>/dev/null			# suppress any crash messages, which 'wait' would print
+e=$?							# get job's exit status from 'wait'
+((!e)) || err_exit "backtick comsub crash/freeze (got status $e$( ((e>128)) && print -n / && kill -l "$e"))"
+kill "$sleep_pid" 2>/dev/null
+
 # ======
 exit $((Errors<125?Errors:125))

Original file line number	Diff line number	Diff line change
`@@ -1803,6 +1803,7 @@ static int job_chksave(register pid_t pid)`
`1803`	`1803`	`{`
`1804`	`1804`	`count = bp->count;`
`1805`	`1805`	`jp = bp->list;`
	`1806`	`+ jpold = 0;`
`1806`	`1807`	`goto again;`
`1807`	`1808`	`}`
`1808`	`1809`	`if(jp)`
Original file line number	Diff line number	Diff line change
`@@ -524,7 +524,10 @@ Sfio_t sh_subshell(Shell_t shp,Shnode_t *t, volatile int flags, int comsub)`
`524`	`524`	`sp->comsub = shp->comsub;`
`525`	`525`	`shp->subshare = comsub==2 \|\| (comsub==1 && sh_isoption(SH_SUBSHARE));`
`526`	`526`	`if(comsub)`
	`527`	`+ {`
`527`	`528`	`shp->comsub = comsub;`
	`529`	`+ job.bktick_waitall = (comsub==1);`
	`530`	`+ }`
`528`	`531`	`if(!comsub \|\| !shp->subshare)`
`529`	`532`	`{`
`530`	`533`	`struct subshell *xp;`
`@@ -655,6 +658,7 @@ Sfio_t sh_subshell(Shell_t shp,Shnode_t *t, volatile int flags, int comsub)`
`655`	`658`	`}`
`656`	`659`	`else`
`657`	`660`	`{`
	`661`	`+ job.bktick_waitall = 0;`
`658`	`662`	`if(comsub!=1 && shp->spid)`
`659`	`663`	`{`
`660`	`664`	`job_wait(shp->spid);`
Original file line number	Diff line number	Diff line change
`@@ -1927,7 +1927,7 @@ int sh_exec(register const Shnode_t *t, int flags)`
`1927`	`1927`	`memset(exitval,0,job.waitall*sizeof(int));`
`1928`	`1928`	`}`
`1929`	`1929`	`else`
`1930`		`- job.waitall \|= !pipejob && sh_isstate(SH_MONITOR);`
	`1930`	`+ job.waitall \|= (job.bktick_waitall \|\| !pipejob && sh_isstate(SH_MONITOR));`
`1931`	`1931`	`job_lock();`
`1932`	`1932`	`nlock++;`
`1933`	`1933`	`do`