From d650c73e55572f910fbf7b1d1663d32b4c00a8c9 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Tue, 1 Feb 2022 03:11:10 +0000 Subject: [PATCH] Fix SIGINT handling for external commands run from scripts Reproducer: $ ksh -c 'bash -c '\''kill -s INT $$'\''; echo "$?, continuing"' Expected result: output "258, continuing"; exit status 0. Actual result: no output; exit status 258. The child process sent SIGINT only to itself and not to the process group, so the parent script was wrongly interrupted. Every shell except ksh93 produces the expected result. ksh93 also gave the expected result before version 2008-01-31 93s+, which introduced the code below. Analysis: The problem is in these lines of code in xec.c, sh_exec(), TFORK case, parent branch of fork: 1649: if(!sh_isstate(SH_MONITOR)) 1650: { 1651: if(!(sh.sigflag[SIGINT]&(SH_SIGFAULT|SH_SIGOFF))) 1652: sh_sigtrap(SIGINT); 1653: sh.trapnote |= SH_SIGIGNORE; 1654: } [...pipe and I/O handling, wait for command to finish...] 1667: if(!sh_isstate(SH_MONITOR)) 1668: { 1669: sh.trapnote &= ~SH_SIGIGNORE; 1700: if(sh.exitval == (SH_EXITSIG|SIGINT)) 1701: kill(sh.current_pid,SIGINT); 1702: } When a user presses Ctrl+C, SIGINT is sent to the entire process group. If job control is fully off (i.e., !sh_isstate(SH_MONITOR)), then the process group includes the parent script. Therefore, in a script such as $ ksh -c 'bash -c '\''read x'\''; echo "$?, continuing"' when the user presses Ctrl+C while bash waits for 'read x' input, the parent ksh script should be interrupted as well. Now, the code above ignores SIGINT while bash is running. (This is done using special-casing in sh_fault() to handle that SH_SIGIGNORE flag for SIGINT.) So, when Ctrl+C interrupts the process group, the parent script is not getting interrupted as it should. To compensate for that, the code then detects, using sh.exitval (the child process' exit status), whether the child process was killed by SIGINT. If so, it simply assumes that the signal was meant for the process group including the parent script, so it reissues SIGINT to itself after unignoring it. But, as we can see from the broken reproducer above, that assumption is not valid. Scripts are perfectly free to send SIGINT to themselves only, and that must work as expected. src/cmd/ksh93/sh/xec.c: sh_exec(): TFORK: parent branch: - Instead of ignoring SIGINT, sigblock() it, which delays handling the signal until sigrelease(). (Note that these are macros defined in src/cmd/ksh93/features/sigfeatures according to OS capabilities.) - This makes reissuing SIGINT redundant, so delete that, which fixes the bug. src/cmd/ksh93/sh/fault.c: - Nothing now sets the SH_SIGIGNORE flag in sh.trapnote, so remove special-casing added in 2008-01-31 93s+. --- NEWS | 4 ++++ src/cmd/ksh93/sh/fault.c | 4 +--- src/cmd/ksh93/sh/xec.c | 8 ++------ src/cmd/ksh93/tests/signal.sh | 21 ++++++++++++++++++--- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index a501ec4fe..b52cc56d3 100644 --- a/NEWS +++ b/NEWS @@ -22,6 +22,10 @@ Any uppercase BUG_* names are modernish shell bug IDs. - In emacs mode, Insert escapes the next character. - In vi mode, Insert will switch the editor to insert mode (like in vim). +- Fixed a bug in SIGINT handling: if a script ran an external command that + interrupted itself (and only itself, not the process group) with SIGINT, + the script that ran the command was also interrupted. + 2022-01-28: - Fixed longstanding job control breakage that occurred on the interactive diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c index 58183c4df..f6b80a94b 100644 --- a/src/cmd/ksh93/sh/fault.c +++ b/src/cmd/ksh93/sh/fault.c @@ -113,8 +113,6 @@ void sh_fault(register int sig) flag = sh.sigflag[sig]&~SH_SIGOFF; if(!trap) { - if(sig==SIGINT && (sh.trapnote&SH_SIGIGNORE)) - return; if(flag&SH_SIGIGNORE) { if(sh.subshell) @@ -386,7 +384,7 @@ void sh_chktrap(void) { register int sig=sh.st.trapmax; register char *trap; - if(!(sh.trapnote&~SH_SIGIGNORE)) + if(!sh.trapnote) sig=0; sh.trapnote &= ~SH_SIGTRAP; /* execute errexit trap first */ diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index cc7712728..d09a2f2eb 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1650,7 +1650,7 @@ int sh_exec(register const Shnode_t *t, int flags) { if(!(sh.sigflag[SIGINT]&(SH_SIGFAULT|SH_SIGOFF))) sh_sigtrap(SIGINT); - sh.trapnote |= SH_SIGIGNORE; + sigblock(SIGINT); } if(sh.pipepid) sh.pipepid = parent; @@ -1665,11 +1665,7 @@ int sh_exec(register const Shnode_t *t, int flags) if(usepipe && tsetio && subdup && unpipe) sh_iounpipe(); if(!sh_isstate(SH_MONITOR)) - { - sh.trapnote &= ~SH_SIGIGNORE; - if(sh.exitval == (SH_EXITSIG|SIGINT)) - kill(sh.current_pid,SIGINT); - } + sigrelease(SIGINT); } if(type&FAMP) { diff --git a/src/cmd/ksh93/tests/signal.sh b/src/cmd/ksh93/tests/signal.sh index d02c2f7c8..87bcf276d 100755 --- a/src/cmd/ksh93/tests/signal.sh +++ b/src/cmd/ksh93/tests/signal.sh @@ -76,6 +76,8 @@ expect=01got_child23 [[ $actual == "$expect" ]] || err_exit 'SIGCHLD not working' \ "(expected $(printf %q "$expect"), got $(printf %q "$actual"))" +# ====== + # begin standalone SIGINT test generation cat > tst <<'!' @@ -243,14 +245,14 @@ chmod +x tst tst-? # end standalone test generation -export PATH=$PATH: +PATH=:$PATH typeset -A expected expected[---]="3-intr" expected[--d]="3-intr" expected[-t-]="3-intr 2-intr 1-intr 1-0258" expected[-td]="3-intr 2-intr 1-intr 1-0258" -expected[x--]="3-intr 2-intr 1-0000" -expected[x-d]="3-intr 2-intr 1-0000" +expected[x--]="3-intr 2-intr" +expected[x-d]="3-intr 2-intr" expected[xt-]="3-intr 2-intr 1-intr 1-0000" expected[xtd]="3-intr 2-intr 1-intr 1-0000" expected[z--]="3-intr 2-intr 1-0000" @@ -263,6 +265,10 @@ tst $SHELL > tst.got while read ops out do [[ $out == ${expected[$ops]} ]] || err_exit "interrupt $ops test failed -- expected '${expected[$ops]}', got '$out'" done < tst.got +unset expected +PATH=${PATH#:} + +# ====== if [[ ${SIG[USR1]} ]] then float s=$SECONDS @@ -553,5 +559,14 @@ got=$( set +x; { "$SHELL" bar; } 2>&1 ) (( (e=$?)==0 )) && [[ $got == "$exp" ]] || err_exit "segfaulting child process:" \ "(expected status 0 and $(printf %q "$exp"), got status $e and $(printf %q "$got"))" +# ====== +# A script that SIGINTs only itself (not the process group) should not cause the parent script to be interrupted +trap '' INT # workaround for old ksh -- ignore SIGINT or the entire test suite gets interrupted +exp='258, continuing' +got=$("$SHELL" -c 'trap + INT; "$SHELL" -c '\''kill -s INT $$'\''; echo "$?, continuing"') +((!(e = $?))) && [[ $got == "$exp" ]] || err_exit "child process interrupting itself interrupts parent" \ + "(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))" +trap - INT + # ====== exit $((Errors<125?Errors:125))