Fix the exec optimisation mess (re: 17ebfbf, 6701bb3, f0b0b67)

McDutchie · McDutchie · commit 88aa369eb8ce · 2022-06-18T21:42:09.000+01:00
This commit supersedes @lijog's Solaris patch 280-23332860 (see 17ebfbf) as this is a more general fix that makes the patch redundant. Of course its associated regression tests stay. Reproducer script: trap 'echo SIGUSR1 received' USR1 sh -c 'kill -s USR1 $PPID' Run as a normal script. Expected behaviour: prints "SIGUSR1 received" Actual behaviour: the shell invoking the script terminates. Oops. As of 6701bb3, ksh again allows an exec-without-fork optimisation for the last command in a script. So the 'sh' command gets the same PID as the script, therefore its parent PID ($PPID) is the invoking script and not the script itself, which has been overwritten in working memory. This shows that, if there are traps set, the exec optimisation is incorrect as the expected process is not signalled. While 6701bb3 reintroduced this problem for scripts, this has always been an issue for certain other situations: forked command substitutions, background subshells, and -c option argument scripts. This commit fixes it in all those cases. In sh_exec(), case TFORK, the optimisation (flagged in no_fork) was only blocked for SIGINT and for the EXIT and ERR pseudosignals. That is wrong. It should be blocked for all signal and pseudosignal traps, except DEBUG (which is run before the command) and SIGKILL and SIGSTOP (which cannot be trapped). (I've also tested the behaviour of other shells. One shell, mksh, never does an exec optimisation, even if no traps are set. I don't know if that is intentional or not. I suppose it is possible that a script might expect to receive a signal without trapping it first, and they could conceivably be affected the same way by this exec optimisation. But the ash variants (e.g. Busybox ash, dash, FreeBSD sh), as well as bash, yash and zsh, all do act like this, so the behaviour is very widespread. This commit makes ksh act like them.) Multiple files: - Remove the sh.errtrap, sh.exittrap and sh.end_fn flags and their associated code from the superseded Solaris patch. src/cmd/ksh93/include/shell.h: - Add a scoped sh.st.trapdontexec flag for sh_exec() to disable exec-without-fork optimisations. It should be in the sh.st scope struct because it needs to be reset in subshell scopes. src/cmd/ksh93/bltins/trap.c: b_trap(): - Set sh.st.trapdontexec if any trap is set and non-empty (an empty trap means ignore the signal, which is inherited by an exec'd process, so the optimisation is fine in that case). - Only clear sh.st.trapdontexec if we're not in a ksh function scope; unlike subshells, ksh functions fall back to parent traps if they don't trap a signal themselves, so a ksh function's parent traps also need to disable the exec optimisation. src/cmd/ksh93/sh/fault.c: sh_sigreset(): - Introduce a new -1 mode for sh_funscope() to use, which acts like mode 0 except it does not clear sh.st.trapdontexec. This avoids clearing sh.st.trapdontexec for ksh functions scopes (see above). - Otherwise, clear sh.st.trapdontexec whenever traps are reset. src/cmd/ksh93/sh/xec.c: check_exec_optimization(): - Consolidate all the exec optimisation logic into this function, including the logic from the no_fork flag in sh_exec()/TFORK. - In the former no_fork flag logic, replace the three checks for SIGINT, ERR and EXIT traps with a single check for the sh.st.trapdontexec flag.
diff --git a/NEWS b/NEWS
@@ -10,6 +10,10 @@ Any uppercase BUG_* names are modernish shell bug IDs.
   the last component command was an external command and the pipeline was
   the last command in a background subshell.
 
+- When any trap except DEBUG, KILL or STOP is set to a non-empty command,
+  the last command in a script or forked subshell will no longer avoid forking
+  before executing; this optimization incorrectly bypassed the traps.
+
 2022-06-15:
 
 - Fixed a bug where converting an indexed array into an associative array in
diff --git a/src/cmd/ksh93/bltins/trap.c b/src/cmd/ksh93/bltins/trap.c
@@ -136,13 +136,6 @@ int	b_trap(int argc,char *argv[],Shbltin_t *context)
 						sh.trapnote = 0;
 
 				}
-				if(sig == SH_ERRTRAP)
-				{
-					if(clear)
-						sh.errtrap = 0;
-					else if(!sh.fn_depth || sh.end_fn)
-						sh.errtrap = 1;
-				}
 				continue;
 			}
 			if(sig > sh.sigmax)
@@ -159,8 +152,6 @@ int	b_trap(int argc,char *argv[],Shbltin_t *context)
 			else if(clear)
 			{
 				sh_sigclear(sig);
-				if(sig == 0)
-					sh.exittrap = 0;
 				if(dflag)
 					signal(sig,SIG_DFL);
 			}
@@ -180,8 +171,39 @@ int	b_trap(int argc,char *argv[],Shbltin_t *context)
 				sh.st.trapcom[sig] = (sh.sigflag[sig]&SH_SIGOFF) ? Empty : sh_strdup(action);
 				if(arg && arg != Empty)
 					free(arg);
-				if(sig == 0 && (!sh.fn_depth || sh.end_fn))
-					sh.exittrap = 1;
+			}
+		}
+		/*
+		 * Set a flag for sh_exec() to disable exec-without-fork optimizations if any trap is set and non-empty.
+		 * (In ksh functions, there may be parent scope traps, so do not reset to 0 if in a ksh function.)
+		 */
+		if(sh.fn_depth==0)
+			sh.st.trapdontexec = 0;
+		if(!sh.st.trapdontexec)
+		{
+			/* EXIT and real signals */
+			for(sig=0; sig<=sh.sigmax; sig++)
+			{
+				/* these cannot be trapped */
+				if(sig==SIGKILL || sig==SIGSTOP)
+					continue;
+				if(sh.st.trapcom[sig] && *sh.st.trapcom[sig])
+				{
+					sh.st.trapdontexec++;
+					break;
+				}
+			}
+		}
+		if(!sh.st.trapdontexec)
+		{
+			/* other pseudosignals -- exclude DEBUG as it is executed before the command */
+			for(sig=0; sig<SH_DEBUGTRAP; sig++)
+			{
+				if(sh.st.trap[sig] && *sh.st.trap[sig])
+				{
+					sh.st.trapdontexec++;
+					break;
+				}
 			}
 		}
 	}
diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h
@@ -204,6 +204,7 @@ struct sh_scoped
 	short		opterror;
 	int		ioset;
 	unsigned short	trapmax;
+	char		trapdontexec;	/* stop exec optimization if any non-DEBUG/SIGKILL/SIGSTOP trap is set and non-empty */
 	char		*trap[SH_DEBUGTRAP+1];
 	char		**otrap;
 	char		**trapcom;
@@ -335,9 +336,9 @@ struct Shell_s
 	int		inuse_bits;
 	struct argnod	*envlist;
 	struct dolnod	*arglist;
-	int		fn_depth;
+	int		fn_depth;	/* scoped ksh-style function call depth */
 	int		fn_reset;
-	int		dot_depth;
+	int		dot_depth;	/* dot-script and POSIX function call depth */
 	int		hist_depth;
 	int		xargmin;
 	int		xargmax;
@@ -381,9 +382,6 @@ struct Shell_s
 	char		*mathnodes;
 	char		*bltin_dir;
 	struct Regress_s*regress;
-	char 		exittrap;
-	char 		errtrap;
-	char 		end_fn;
 #if SHOPT_FILESCAN
 	char		*cur_line;
 #endif
diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c
@@ -315,11 +315,17 @@ void	sh_sigdone(void)
  * Restore to default signals
  * Free the trap strings if mode is non-zero
  * If mode>1 then ignored traps cause signal to be ignored 
+ * If mode==-1 we're entering a new function scope in sh_funscope()
  */
 void	sh_sigreset(register int mode)
 {
 	register char	*trap;
 	register int 	flag, sig=sh.st.trapmax;
+	/* do not reset sh.st.trapdontexec in a new ksh function scope as parent traps will still be active */
+	if(mode < 0)
+		mode = 0;
+	else
+		sh.st.trapdontexec = 0;
 	while(sig-- > 0)
 	{
 		if(trap=sh.st.trapcom[sig])
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
@@ -1527,9 +1527,6 @@ Shell_t *sh_init(register int argc,register char *argv[], Shinit_f userinit)
 	astintercept(&sh.bltindata,1);
 	if(sh.userinit=userinit)
 		(*userinit)(&sh, 0);
-	sh.exittrap = 0;
-	sh.errtrap = 0;
-	sh.end_fn = 0;
 	error_info.exit = sh_exit;
 #ifdef BUILD_DTKSH
 	{
@@ -1714,9 +1711,6 @@ int sh_reinit(char *argv[])
 	sh.inpipe = sh.outpipe = 0;
 	job_clear();
 	job.in_critical = 0;
-	sh.exittrap = 0;
-	sh.errtrap = 0;
-	sh.end_fn = 0;
 	/* update ${.sh.pid}, $$, $PPID */
 	sh.ppid = sh.current_pid;
 	sh.current_pid = sh.pid = getpid();
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
@@ -467,9 +467,6 @@ static void	exfile(register Sfio_t *iop,register int fno)
 	error_info.line = 1;
 	sh.inlineno = 1;
 	sh.binscript = 0;
-	sh.exittrap = 0;
-	sh.errtrap = 0;
-	sh.end_fn = 0;
 	if(sfeof(iop))
 		goto eof_or_error;
 	/* command loop */
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
@@ -909,10 +909,17 @@ static Namval_t *enter_namespace(Namval_t *nsp)
 /*
  * Check whether to execve(2) the final command or make its redirections permanent.
  */
-static int check_exec_optimization(struct ionod *iop)
+static int check_exec_optimization(int type, int execflg, int execflg2, struct ionod *iop)
 {
-	if(sh.subshell || sh.exittrap || sh.errtrap)
+	if(type&(FAMP|FPOU)
+	|| !(execflg && sh.fn_depth==0 || execflg2)
+	|| sh.st.trapdontexec
+	|| sh.subshell
+	|| ((struct checkpt*)sh.jmplist)->mode==SH_JMPEVAL
+	|| (pipejob && (sh_isstate(SH_MONITOR) || sh_isoption(SH_PIPEFAIL) || sh_isstate(SH_TIMING))))
+	{
 		return(0);
+	}
 	/* '<>;' (IOREWRITE) redirections are incompatible with exec */
 	while(iop && !(iop->iofile & IOREWRITE))
 		iop = iop->ionxt;
@@ -1133,7 +1140,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 				int tflags = 1;
 				if(np && nv_isattr(np,BLT_DCL))
 					tflags |= 2;
-				if(execflg && !check_exec_optimization(io))
+				if(execflg && !check_exec_optimization(type,execflg,execflg2,io))
 					execflg = 0;
 				if(argn==0)
 				{
@@ -1506,15 +1513,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 			int pipes[3];
 			if(sh.subshell)
 				sh_subtmpfile();
-			no_fork = !(type&(FAMP|FPOU))
-			&& !sh.subshell
-			&& !(sh.st.trapcom[SIGINT] && *sh.st.trapcom[SIGINT])
-			&& !sh.st.trapcom[0]
-			&& !sh.st.trap[SH_ERRTRAP]
-			&& ((struct checkpt*)sh.jmplist)->mode!=SH_JMPEVAL
-			&& (execflg && sh.fn_depth==0 || execflg2)
-			&& !(pipejob && (sh_isstate(SH_MONITOR) || sh_isoption(SH_PIPEFAIL) || sh_isstate(SH_TIMING)));
-			if(no_fork)
+			if(no_fork = check_exec_optimization(type,execflg,execflg2,t->fork.forkio))
 				job.parent=parent=0;
 			else
 			{
@@ -1799,7 +1798,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 			jmpval = sigsetjmp(buffp->buff,0);
 			if(jmpval==0)
 			{
-				if(execflg && !check_exec_optimization(t->fork.forkio))
+				if(execflg && !check_exec_optimization(type,execflg,execflg2,t->fork.forkio))
 				{
 					execflg = 0;
 					flags &= ~sh_state(SH_NOFORK);
@@ -3100,7 +3099,7 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 	}
 	if(!fun && sh_isoption(SH_FUNCTRACE) && sh.st.trap[SH_DEBUGTRAP] && *sh.st.trap[SH_DEBUGTRAP])
 		save_debugtrap = sh_strdup(sh.st.trap[SH_DEBUGTRAP]);
-	sh_sigreset(0);
+	sh_sigreset(-1);
 	if(save_debugtrap)
 		sh.st.trap[SH_DEBUGTRAP] = save_debugtrap;
 	argsav = sh_argnew(argv,&saveargfor);
@@ -3117,8 +3116,6 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 		nv_putval(SH_PATHNAMENOD,sh.st.filename,NV_NOFREE);
 		nv_putval(SH_FUNNAMENOD,sh.st.funname,NV_NOFREE);
 	}
-	if((execflg & sh_state(SH_NOFORK)))
-		sh.end_fn = 1;
 	jmpval = sigsetjmp(buffp->buff,0);
 	if(jmpval == 0)
 	{
@@ -3169,7 +3166,6 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 	sh.st = *prevscope;
 	sh.topscope = (Shscope_t*)prevscope;
 	nv_getval(sh_scoped(IFSNOD));
-	sh.end_fn = 0;
 	if(nsig)
 	{
 		for (isig = 0; isig < nsig; ++isig)
diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh
@@ -928,14 +928,77 @@ AIX | SunOS)
 esac
 
 # ======
+# Test exec optimization of last command in script or subshell
+
 (
-	ulimit -t unlimited 2>/dev/null
+	ulimit -t unlimited 2>/dev/null  # fork subshell
 	print "${.sh.pid:-$("$SHELL" -c 'echo "$PPID"')}"  # fallback for pre-93u+m ksh without ${.sh.pid}
 	"$SHELL" -c 'print "$$"'
 ) >out
 pid1= pid2=
 { read pid1 && read pid2; } <out && let "pid1 == pid2" \
 || err_exit "last command in forked subshell not exec-optimized ($pid1 != $pid2)"
 
+got=$(
+	ulimit -t unlimited 2>/dev/null  # fork subshell
+	print "${.sh.pid:-$("$SHELL" -c 'echo "$PPID"')}"  # fallback for pre-93u+m ksh without ${.sh.pid}
+	"$SHELL" -c 'print "$$"'
+)
+pid1= pid2=
+{ read pid1 && read pid2; } <<<$got && let "pid1 == pid2" \
+|| err_exit "last command in forked comsub not exec-optimized ($pid1 != $pid2)"
+
+cat >script <<\EOF
+echo $$
+sh -c 'echo $$'
+EOF
+"$SHELL" script >out
+pid1= pid2=
+{ read pid1 && read pid2; } <out && let "pid1 == pid2" \
+|| err_exit "last command in script not exec-optimized ($pid1 != $pid2)"
+
+for sig in EXIT ERR ${ kill -l; }
+do
+	case $sig in
+	KILL | STOP)
+		# cannot be trapped
+		continue ;;
+	esac
+
+	# the following is tested in a background subshell because ksh before 2022-06-18 didn't
+	# do exec optimization on the last external command in a forked non-background subshell
+	(
+		trap + "$sig"  # unadvertised (still sort of broken) feature: unignore signal
+		trap : "$sig"
+		print "${.sh.pid:-$("$SHELL" -c 'echo "$PPID"')}"  # fallback for pre-93u+m ksh without ${.sh.pid}
+		"$SHELL" -c 'print "$$"'
+	) >out &
+	wait
+	pid1= pid2=
+	{ read pid1 && read pid2; } <out && let "pid1 != pid2" \
+	|| err_exit "last command in forked subshell exec-optimized in spite of $sig trap ($pid1 == $pid2)"
+
+	got=$(
+		ulimit -t unlimited 2>/dev/null  # fork subshell
+		trap + "$sig"  # unadvertised (still sort of broken) feature: unignore signal
+		trap : "$sig"
+		print "${.sh.pid:-$("$SHELL" -c 'echo "$PPID"')}"  # fallback for pre-93u+m ksh without ${.sh.pid}
+		"$SHELL" -c 'print "$$"'
+	)
+	pid1= pid2=
+	{ read pid1 && read pid2; } <<<$got && let "pid1 != pid2" \
+	|| err_exit "last command in forked comsub exec-optimized in spite of $sig trap ($pid1 == $pid2)"
+
+	cat >script <<-EOF
+	trap ":" $sig
+	echo \$\$
+	sh -c 'echo \$\$'
+	EOF
+	"$SHELL" script >out
+	pid1= pid2=
+	{ read pid1 && read pid2; } <out && let "pid1 != pid2" \
+	|| err_exit "last command in script exec-optimized in spite of $sig trap ($pid1 == $pid2)"
+done
+
 # ======
 exit $((Errors<125?Errors:125))
