[RFC PATCH] Allow trap to override permanently-ignored signals in background shells

[RFC PATCH] Allow trap to override permanently-ignored signals in background shells

[Johannes Altmanninger]

TL;DR: I think this job should exit on Control-C.

	( trap - INT; sleep inf ) &

With shell=bash or shell=zsh this works with below script.
Meanwhile, shell=dash fails

	shell=dash
	set -e
	n=123
	pkill -f "sleep.$n" ||:
	pid=$(setsid "$shell" -c "( trap - INT; sleep $n ) </dev/null >/dev/null 2>&1 & echo \$\$")
	sleep 1
	kill -INT -$pid
	ps aux | grep "[s]leep.$n"

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.

and continues:

> 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 whether the trap builtin is supposed to override
the ignore. Intuitively, I'd say yes, and the Bash maintainer seems
to agree responding to a related bug report about Bash functions[**]

> 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)

Attached patch works for me though it's barely thought-through or
tested. Happy to take it to completion.

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

{{{

Backstory:

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

	mkfifo /tmp/fifo
	(
		sleep inf # in practice "make"
	) >/tmp/fifo 2>&1 &

On Control-C, the editor traditionally sends SIGINT to the process
group. Bash documentation says[2]:

> When job control is not in effect, asynchronous commands ignore
> SIGINT and SIGQUIT in addition to these inherited handlers.

This means that the "sleep inf" process ignores SIGINT.
We have worked around this fact by sending SIGTERM instead[3].

If the forked process sets a SIGINT handler of any form, for example
by using "signal(SIGINT, SIG_DFL)", it will no longer ignore SIGINT.

It seems reasonable to give (sub)shells the same powers, via the
"trap" builtin. This would allow me to change the above subshell to

	(
		trap - INT
		sleep inf
	) >/tmp/fifo 2>&1 &

to have it be terminated by SIGINT.

In general, I wonder if SIGINT is only for actual shells, and SIGTERM
is the signal to use in our situation.

[1]: https://kakoune.org/
[2]: https://www.gnu.org/software/bash/manual/html_node/Signals.html
[3]: https://lists.sr.ht/~mawww/kakoune/%3C20240307135831.1967826-3-aclopte@gmail.com%3E

}}}
---
 src/trap.c | 12 +++++++++---
 src/trap.h |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/trap.c b/src/trap.c
index cd84814..a6778e8 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -159,7 +159,7 @@ trapcmd(int argc, char **argv)
 		}
 		trap[signo] = action;
 		if (signo != 0)
-			setsignal(signo);
+			setsignal_maybe_user(signo, 1);
 		INTON;
 		ap++;
 	}
@@ -175,6 +175,12 @@ trapcmd(int argc, char **argv)
 
 void
 setsignal(int signo)
+{
+    setsignal_maybe_user(signo, 0);
+}
+
+void
+setsignal_maybe_user(int signo, int user)
 {
 	int action;
 	int lvforked;
@@ -234,7 +240,7 @@ setsignal(int signo)
 		}
 		if (act.sa_handler == SIG_IGN) {
 			if (mflag && (signo == SIGTSTP ||
-			     signo == SIGTTIN || signo == SIGTTOU)) {
+			              signo == SIGTTIN || signo == SIGTTOU)) {
 				tsig = S_IGN;	/* don't hard ignore these */
 			} else
 				tsig = S_HARD_IGN;
@@ -242,7 +248,7 @@ setsignal(int signo)
 			tsig = S_RESET;	/* force to be set */
 		}
 	}
-	if (tsig == S_HARD_IGN || tsig == action)
+	if ((!user && tsig == S_HARD_IGN) || tsig == action)
 		return;
 	switch (action) {
 	case S_CATCH:
diff --git a/src/trap.h b/src/trap.h
index beaf660..595992c 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -43,6 +43,7 @@ extern volatile sig_atomic_t gotsigchld;
 
 int trapcmd(int, char **);
 void setsignal(int);
+void setsignal_maybe_user(int, int);
 void ignoresig(int);
 void onsig(int);
 void dotrap(void);
-- 
2.44.0.rc1.17.g3e0d3cd5c7

Re: [RFC PATCH] Allow trap to override permanently-ignored signals in background shells

[Harald van Dijk]

Hi,

On 08/03/2024 11:01, Johannes Altmanninger wrote:
> TL;DR: I think this job should exit on Control-C.
> 
> 	( trap - INT; sleep inf ) &

The TL;DR is oversimplified: Control-C would not result in SIGINT being 
sent to the sleep command, because it runs in the background.

Your fuller version, I agree, I think that is a bug in dash. There are 
some more known bugs in its handling of the trap command in subshells 
that require a bigger rework (particularly the one where, despite traps 
being reset in subshells, the 'trap' command should print the parent 
shell's traps if called in a subshell) that maybe should be looked at at 
the same time.

> In general, I wonder if SIGINT is only for actual shells, and SIGTERM
> is the signal to use in our situation.

SIGINT is not limited to shells. At first glance, sending SIGINT to 
another process or process group upon receipt of SIGINT, because that 
other process or process group is what it was intended for, seems like 
an appropriate use to me.

Cheers,
Harald van Dijk

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

[Johannes Altmanninger]

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(-)

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;
 }
 
 
-- 
2.44.0.368.gc75fd8d815

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

[Jilles Tjoelker]

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.

-- 
Jilles Tjoelker

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

[Johannes Altmanninger]

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.

[PATCH v3] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Johannes Altmanninger]

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.

Builtin [trap] has this requirement:

> 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.

Apparently this only applies to signals that were inherited as ignored,
not to the special case of SIGINT/SIGQUIT begin ignored in asynchronous
subshells.

Make it so. This means that either of

	trap - INT; trap - QUIT
	set -i

in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.

[Signals]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
[trap]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap

{{{ 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"' makes 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/trap.c b/src/trap.c
index cd84814..dbf81ea 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -272,7 +272,7 @@ ignoresig(int signo)
 		signal(signo, SIG_IGN);
 	}
 	if (!vforked)
-		sigmode[signo - 1] = S_HARD_IGN;
+		sigmode[signo - 1] = S_IGN;
 }
 
 
-- 
2.44.0.368.gc75fd8d815

Re: [PATCH v3] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Herbert Xu]

On Fri, Mar 29, 2024 at 04:39:01PM +0100, Johannes Altmanninger wrote:
>
> diff --git a/src/trap.c b/src/trap.c
> index cd84814..dbf81ea 100644
> --- a/src/trap.c
> +++ b/src/trap.c
> @@ -272,7 +272,7 @@ ignoresig(int signo)
>  		signal(signo, SIG_IGN);
>  	}
>  	if (!vforked)
> -		sigmode[signo - 1] = S_HARD_IGN;
> +		sigmode[signo - 1] = S_IGN;

This is buggy if sigmode is already S_HARD_IGN.  You can fix
this by moving the if statement inside the previous one.

Please also add a comment stating that sigmode has already been
initialised by setinteractive, as otherwise we may also lose a
hard ignore.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v4] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Johannes Altmanninger]

Unlike in Bash or Zsh, this asynchronous job ignores SIGINT, despite
builtin trap explicitly resetting the SIGINT handler.

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

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.

Builtin [trap] has this requirement:

> 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.

Apparently this only applies to signals that were inherited as ignored,
not to the special case of SIGINT/SIGQUIT begin ignored in asynchronous
subshells.

Make it so. This means that either of

	trap - INT; trap - QUIT
	set -i

in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.

[Signals]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
[trap]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap

{{{ 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"' makes 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/trap.c b/src/trap.c
index f871656..90e9607 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -296,11 +296,11 @@ setsignal(int signo)
 void
 ignoresig(int signo)
 {
-	if (sigmode[signo - 1] != S_IGN && sigmode[signo - 1] != S_HARD_IGN) {
-		signal(signo, SIG_IGN);
-	}
+	if (sigmode[signo - 1] == S_IGN || sigmode[signo - 1] == S_HARD_IGN)
+        	return;
+	signal(signo, SIG_IGN);
 	if (!vforked)
-		sigmode[signo - 1] = S_HARD_IGN;
+		sigmode[signo - 1] = S_IGN;
 }
 
 
-- 
2.45.1.190.g19fe900cfc

Re: [PATCH v3] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Johannes Altmanninger]

On Sun, Apr 07, 2024 at 07:18:52PM +0800, Herbert Xu wrote:
> On Fri, Mar 29, 2024 at 04:39:01PM +0100, Johannes Altmanninger wrote:
> >
> > diff --git a/src/trap.c b/src/trap.c
> > index cd84814..dbf81ea 100644
> > --- a/src/trap.c
> > +++ b/src/trap.c
> > @@ -272,7 +272,7 @@ ignoresig(int signo)
> >  		signal(signo, SIG_IGN);
> >  	}
> >  	if (!vforked)
> > -		sigmode[signo - 1] = S_HARD_IGN;
> > +		sigmode[signo - 1] = S_IGN;
> 
> This is buggy if sigmode is already S_HARD_IGN.  You can fix
> this by moving the if statement inside the previous one.
> 
> Please also add a comment stating that sigmode has already been
> initialised by setinteractive, as otherwise we may also lose a
> hard ignore.

I'm not really following the last part;
maybe it's no longer relevant with the bug fix.
Note that it works the same whether "set -i" or "trap - INT" is used,
also in noninteractive shells like

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

Re: [PATCH v3] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Herbert Xu]

On Sat, May 18, 2024 at 10:43:12AM +0200, Johannes Altmanninger wrote:
>
> I'm not really following the last part;
> maybe it's no longer relevant with the bug fix.
> Note that it works the same whether "set -i" or "trap - INT" is used,
> also in noninteractive shells like
> 
> 	dash -c '( trap - INT; sleep inf ) & read _'

All I meant was that if you call ignoresig before sigmode itself
is initialised it will do the wrong thing if the signal was supposed
to be a hard ignore.

However, this can't happen because we only call ignoresig for
SIGINT and SIGQUIT, and for those two signals, ignoresig is
always called after setinteractive which will initialise the
sigmode array to S_HARD_IGN if necessary.

I'll add a comment when applying your patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v4] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

[Herbert Xu]

On Sat, May 18, 2024 at 10:38:30AM +0200, 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 ) & read _'
> 
> 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.
> 
> Builtin [trap] has this requirement:
> 
> > 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.
> 
> Apparently this only applies to signals that were inherited as ignored,
> not to the special case of SIGINT/SIGQUIT begin ignored in asynchronous
> subshells.
> 
> Make it so. This means that either of
> 
> 	trap - INT; trap - QUIT
> 	set -i
> 
> in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.
> 
> [Signals]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
> [trap]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap
> 
> {{{ 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"' makes 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt