diff mbox series

[v2] Allow trap to un-ignore SIGINT in asynchronous subshells

Message ID 20240329112419.1571653-1-aclopte@gmail.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series [v2] Allow trap to un-ignore SIGINT in asynchronous subshells | expand

Commit Message

Johannes Altmanninger March 29, 2024, 11:24 a.m. UTC 
Unlike in Bash or Zsh, this asynchronous job ignores SIGINT, despite
builtin trap explicitly resetting the SIGINT handler.

	dash -c '( trap - INT; sleep inf ) &'

POSIX[*] Section 2.11 on Signals and Error Handling says about
background execution:

> If job control is disabled (see the description of set -m) when
> the shell executes an asynchronous list, the commands in the list
> shall inherit from the shell a signal action of ignored (SIG_IGN)
> for the SIGINT and SIGQUIT signals.  In all other cases, commands
> executed by the shell shall inherit the same signal actions as those
> inherited by the shell from its parent unless a signal action is
> modified by the trap special built-in (see trap)

It is not clear to me from this description whether the trap builtin is
supposed to un-ignore SIGINT and SIGQUIT in the above asynchronous job.
I think yes because processes like "sh -c 'trap ...'"  can already
do that, so why treat subshells differently.  The Bash maintainer
seems to agree when responding to a related bug report[**]
although that one is not about subshells specifically.

> The issue is that the processes in this list have to ignore SIGINT
> [...] but they have to be allowed to use trap to change the signal
> dispositions (POSIX interp 751)

This issue exists because we "hard-ignore" (meaning ignore
permanently) SIGINT and SIGQUIT in backgrounded subshells; I'm
not sure why.  Git blame is not helpful since this existed since
the initial Git import. I failed to find a test suite; no luck at
http://www.opengroup.org/testing/testsuites/vscpcts2003.htm where I
get SSL errors.

Since I don't see any reason for hard-ignore logic, remove it
altogether.  This means that either of

	trap - INT; trap - QUIT
	set -i

in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.
I did not find other behavior changes but maybe there is a good reason
for S_HARD_IGN?

[*]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
[**]: https://lists.gnu.org/archive/html/bug-bash/2023-01/msg00050.html

{{{ Test cases:

shell=src/dash

set -e

SubshellWith() {
	parent_pid=$(setsid "$shell" -c "( $1; sleep 99 ) </dev/null >/dev/null 2>&1 & echo \$\$")
	sleep 1
	subshell_pid=$(ps -o pid= -$parent_pid | tail -n 1)
}

trap 'kill -TERM -$parent_pid 2>/dev//null ||:' EXIT # Tear down after a failure.

echo Scenario 0: '"set -i"' maks a subshell un-ignore SIGINT.
SubshellWith 'set -i'
kill -INT $subshell_pid
! ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

echo Scenario 1: resetting SIGINT handler.
SubshellWith 'trap - INT'
kill -INT -$parent_pid # kill the whole process group since that's the my use case
! ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

echo Scenario 2: ignoring SIGINT.
SubshellWith 'trap "" INT'
kill -INT $subshell_pid
ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

}}}

{{{ Backstory/motivation:

The Kakoune[1] editor likes to run noninteractive shell commands that
boil down to

	mkfifo /tmp/fifo
	(
		trap - INT
		make
	) >/tmp/fifo 2>&1 &

On Control-C, the editor sends SIGINT to its process group,
which should terminate the subshell running make[2].

We experimented with sending SIGTERM instead but found issues,
specifically if the editor is invoked (without exec) from a wrapper
script, sending SIGTERM to the whole process group would kill the
wrapper script, which in turn makes it send SIGTERM to the editor,
which then terminates.

[1]: https://kakoune.org/
[2]: https://lists.sr.ht/~mawww/kakoune/%3C20240307135831.1967826-3-aclopte@gmail.com%3E

}}}
---
 src/trap.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Jilles Tjoelker March 29, 2024, 1:50 p.m. UTC | #1 
On Fri, Mar 29, 2024 at 12:24:00PM +0100, Johannes Altmanninger wrote:
> Unlike in Bash or Zsh, this asynchronous job ignores SIGINT, despite
> builtin trap explicitly resetting the SIGINT handler.

> 	dash -c '( trap - INT; sleep inf ) &'

> POSIX[*] Section 2.11 on Signals and Error Handling says about
> background execution:

> > If job control is disabled (see the description of set -m) when
> > the shell executes an asynchronous list, the commands in the list
> > shall inherit from the shell a signal action of ignored (SIG_IGN)
> > for the SIGINT and SIGQUIT signals.  In all other cases, commands
> > executed by the shell shall inherit the same signal actions as those
> > inherited by the shell from its parent unless a signal action is
> > modified by the trap special built-in (see trap)

> It is not clear to me from this description whether the trap builtin is
> supposed to un-ignore SIGINT and SIGQUIT in the above asynchronous job.
> I think yes because processes like "sh -c 'trap ...'"  can already
> do that, so why treat subshells differently.  The Bash maintainer
> seems to agree when responding to a related bug report[**]
> although that one is not about subshells specifically.

> > The issue is that the processes in this list have to ignore SIGINT
> > [...] but they have to be allowed to use trap to change the signal
> > dispositions (POSIX interp 751)

> This issue exists because we "hard-ignore" (meaning ignore
> permanently) SIGINT and SIGQUIT in backgrounded subshells; I'm
> not sure why.  Git blame is not helpful since this existed since
> the initial Git import. I failed to find a test suite; no luck at
> http://www.opengroup.org/testing/testsuites/vscpcts2003.htm where I
> get SSL errors.

> Since I don't see any reason for hard-ignore logic, remove it
> altogether.  This means that either of

> 	trap - INT; trap - QUIT
> 	set -i

> in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.
> I did not find other behavior changes but maybe there is a good reason
> for S_HARD_IGN?

> [*]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
> [**]: https://lists.gnu.org/archive/html/bug-bash/2023-01/msg00050.html

There is definitely a good reason for S_HARD_IGN: it implements the
following requirement of the trap builtin (XCU 2.14 Special Built-In
Utilities -> trap):

] Signals that were ignored on entry to a non-interactive shell cannot
] be trapped or reset, although no error need be reported when
] attempting to do so. An interactive shell may reset or catch signals
] ignored on entry.

The change that should be made is that the automatic ignore of SIGINT
and SIGQUIT in background subshells should not use hard ignore. Hard
ignore still applies if the shell inherits ignored signals from its
parent.
Johannes Altmanninger March 29, 2024, 3:02 p.m. UTC | #2 
On Fri, Mar 29, 2024 at 02:50:39PM +0100, Jilles Tjoelker wrote:
> On Fri, Mar 29, 2024 at 12:24:00PM +0100, Johannes Altmanninger wrote:
> > [*]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
> > [**]: https://lists.gnu.org/archive/html/bug-bash/2023-01/msg00050.html
> 
> There is definitely a good reason for S_HARD_IGN: it implements the
> following requirement of the trap builtin (XCU 2.14 Special Built-In
> Utilities -> trap):
> 
> ] Signals that were ignored on entry to a non-interactive shell cannot
> ] be trapped or reset, although no error need be reported when
> ] attempting to do so.

The wording in [*] is extremely confusing.  To me, "If job control
is disabled (see the description of set -m) when the shell executes
an asynchronous list, the commands in the list shall inherit from
the shell a signal action of ignored (SIG_IGN) for the SIGINT and
SIGQUIT signals." sounds like those signals are ignored on entry,
so it sounds like "cannot be trapped or reset" applies.

I guess we are dealing with two different sets of inherited signal
handlers, the specific SIGINT/SIGQUIT for asynchronous subshells,
and the others that were inherited normally.

> ] An interactive shell may reset or catch signals ] ignored on entry.
> 
> The change that should be made is that the automatic ignore of SIGINT
> and SIGQUIT in background subshells should not use hard ignore.
> Hard ignore still applies if the shell inherits ignored signals
> from its parent.

Oh, I had thought, ignoresig(SIGINT) and ignoresig(SIGQUIT) on
background subshell entry were our only uses of S_HARD_IGN.
So this means that this logic from setsignal()

	if (act.sa_handler == SIG_IGN) {
		if (mflag && (signo == SIGTSTP ||
		     signo == SIGTTIN || signo == SIGTTOU)) {
			tsig = S_IGN;	/* don't hard ignore these */
		} else
			tsig = S_HARD_IGN;
	}

interprets all inherited-as-ignored signals as being hard-ignored,
but that doesn't include SIGINT/SIGQUIT because they were inherited
in a different way, via ignoresig()

Will fix.
diff mbox series

Patch

diff --git a/src/trap.c b/src/trap.c
index cd84814..d80fdde 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -55,14 +55,12 @@ 
 /*
  * Sigmode records the current value of the signal handlers for the various
  * modes.  A value of zero means that the current handler is not known.
- * S_HARD_IGN indicates that the signal was ignored on entry to the shell,
  */
 
 #define S_DFL 1			/* default signal handling (SIG_DFL) */
 #define S_CATCH 2		/* signal is caught */
 #define S_IGN 3			/* signal is ignored (SIG_IGN) */
-#define S_HARD_IGN 4		/* signal is ignored permenantly */
-#define S_RESET 5		/* temporary - to reset a hard ignored sig */
+#define S_RESET 4		/* temporary - to reset an ignored sig */
 
 
 /* trap handler commands */
@@ -233,16 +231,12 @@  setsignal(int signo)
 			return;
 		}
 		if (act.sa_handler == SIG_IGN) {
-			if (mflag && (signo == SIGTSTP ||
-			     signo == SIGTTIN || signo == SIGTTOU)) {
-				tsig = S_IGN;	/* don't hard ignore these */
-			} else
-				tsig = S_HARD_IGN;
+			tsig = S_IGN;
 		} else {
 			tsig = S_RESET;	/* force to be set */
 		}
 	}
-	if (tsig == S_HARD_IGN || tsig == action)
+	if (tsig == action)
 		return;
 	switch (action) {
 	case S_CATCH:
@@ -268,11 +262,11 @@  setsignal(int signo)
 void
 ignoresig(int signo)
 {
-	if (sigmode[signo - 1] != S_IGN && sigmode[signo - 1] != S_HARD_IGN) {
+	if (sigmode[signo - 1] != S_IGN) {
 		signal(signo, SIG_IGN);
 	}
 	if (!vforked)
-		sigmode[signo - 1] = S_HARD_IGN;
+		sigmode[signo - 1] = S_IGN;
 }