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

Commit Message

Johannes Altmanninger March 8, 2024, 11:01 a.m. UTC

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/[e-mail]%3E

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

Comments

Harald van Dijk March 8, 2024, 11:48 a.m. UTC | #1

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

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