From 88aa369eb8ceeae489e65a41f14429721744bdbf Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 18 Jun 2022 16:44:30 +0100 Subject: [PATCH] Fix the exec optimisation mess (re: 17ebfbf6, 6701bb30, f0b0b67d) This commit supersedes @lijog's Solaris patch 280-23332860 (see 17ebfbf6) 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 6701bb30, 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 6701bb30 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. --- NEWS | 4 +++ src/cmd/ksh93/bltins/trap.c | 44 ++++++++++++++++++------ src/cmd/ksh93/include/shell.h | 8 ++--- src/cmd/ksh93/sh/fault.c | 6 ++++ src/cmd/ksh93/sh/init.c | 6 ---- src/cmd/ksh93/sh/main.c | 3 -- src/cmd/ksh93/sh/xec.c | 30 +++++++--------- src/cmd/ksh93/tests/basic.sh | 65 ++++++++++++++++++++++++++++++++++- 8 files changed, 123 insertions(+), 43 deletions(-) diff --git a/NEWS b/NEWS index a6c2445e7e67..daf09f9d3699 100644 --- 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 index 76e134de8f69..493aa5a2f928 100644 --- 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; sig1 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 index 6e4ed78e8ee5..fb84480ecd3b 100644 --- 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 index 0f6210e22708..8f43b418818a 100644 --- 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 index a7f093c151b9..d2707d9cb83a 100644 --- 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 index 31fb6336d135..59449651db39 100755 --- a/src/cmd/ksh93/tests/basic.sh +++ b/src/cmd/ksh93/tests/basic.sh @@ -928,8 +928,10 @@ 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 @@ -937,5 +939,66 @@ pid1= pid2= { read pid1 && read pid2; } /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 & + wait + pid1= pid2= + { read pid1 && read pid2; } /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; }