Module Name: src
Committed By: riastradh
Date: Thu Mar 13 12:48:22 UTC 2025
Modified Files:
src/sys/kern: kern_sig.c
src/tests/lib/libc/gen/execve: t_execve.c
src/tests/lib/libc/gen/posix_spawn: t_spawn.c
Log Message:
execve(2), posix_spawn(2): Don't flush _all_ pending signals.
We need only flush those pending signals whose dispositions have been
reset to the default action when that action is to ignore them --
e.g., if the parent had a signal handler function for SIGCHLD or
SIGWINCH, this is reset to the default disposition, which is to
ignore the signal, so any pending SIGCHLD or SIGWINCH need to be
flushed.
And we have logic to do this already in execsigs(9), via
sigclearset(9), which clears the specified set of signals:
402 sigemptyset(&tset);
403 for (signo = 1; signo < NSIG; signo++) {
404 if (sigismember(&p->p_sigctx.ps_sigcatch, signo)) {
405 prop = sigprop[signo];
406 if (prop & SA_IGNORE) {
407 if ((prop & SA_CONT) == 0)
408
sigaddset(&p->p_sigctx.ps_sigignore,
409 signo);
410 sigaddset(&tset, signo);
411 }
412 SIGACTION_PS(ps, signo).sa_handler = SIG_DFL;
...
420 sigclearall(p, &tset, &kq);
https://nxr.netbsd.org/xref/src/sys/kern/kern_sig.c?r=1.409#394
But back in 2003, when ksiginfo_t was introduced, before that logic
was written, we sprouted an exithook to clear _all_ the signals (and,
more importantly for the time, free the ksiginfo_t records to avoid
leaking memory) -- and we wired it up as an _exechook_ too:
+/*
+ * free all pending ksiginfo on exit
+ */
+static void
+ksiginfo_exithook(struct proc *p, void *v)
+{
+ ksiginfo_t *ksi, *hp = p->p_sigctx.ps_siginfo;
+
+ if (hp == NULL)
+ return;
+ for (;;) {
+ pool_put(&ksiginfo_pool, ksi);
+ if ((ksi = ksi->ksi_next) == hp)
+ break;
+ }
+}
...
+ exithook_establish(ksiginfo_exithook, NULL);
+ exechook_establish(ksiginfo_exithook, NULL);
https://mail-index.netbsd.org/source-changes/2003/09/14/msg133910.html
(The first iteration of ksiginfo_exithook had another bug, of course!
But it was soon fixed; that's not the issue here.)
Later, during the newlock2 branch, sigclearall got added for execsigs
to free only the ksiginfo_t records for those signals whose
disposition is being reset to a default action of ignoring the
signal:
void
execsigs(struct proc *p)
{
...
+ sigset_t tset;
...
- for (signum = 1; signum < NSIG; signum++) {
- if (sigismember(&p->p_sigctx.ps_sigcatch, signum)) {
- prop = sigprop[signum];
+ sigemptyset(&tset);
+ for (signo = 1; signo < NSIG; signo++) {
+ if (sigismember(&p->p_sigctx.ps_sigcatch, signo)) {
+ prop = sigprop[signo];
if (prop & SA_IGNORE) {
if ((prop & SA_CONT) == 0)
sigaddset(&p->p_sigctx.ps_sigignore,
- signum);
- sigdelset(&p->p_sigctx.ps_siglist, signum);
+ signo);
+ sigaddset(&tset, signo);
...
}
+ sigclearall(p, &tset);
https://mail-index.netbsd.org/source-changes/2006/10/21/msg176390.html
And the _exithook_ was removed somewhere along the way in the
newlock2 branch (in favour of simply calling sigclearall in exit1),
but the _exechook_ remained:
-static void ksiginfo_exithook(struct proc *, void *);
+static void ksiginfo_exechook(struct proc *, void *);
...
- exithook_establish(ksiginfo_exithook, NULL);
- exechook_establish(ksiginfo_exithook, NULL);
+ exechook_establish(ksiginfo_exechook, NULL);
...
/*
- * ksiginfo_exithook:
+ * ksiginfo_exechook:
*
- * Free all pending ksiginfo entries from a process on exit.
+ * Free all pending ksiginfo entries from a process on exec.
* Additionally, drain any unused ksiginfo structures in the
* system back to the pool.
+ *
+ * XXX This should not be a hook, every process has signals.
*/
static void
-ksiginfo_exithook(struct proc *p, void *v)
+ksiginfo_exechook(struct proc *p, void *v)
{
https://mail-index.netbsd.org/source-changes/2007/02/05/msg180796.html
The symptom of this mistake is that a signal delivered _during_
execve(2) may be simply discarded, even if it should be caught and
cause the process to terminate.
On the bright side, isn't it a nice feeling when you can solve
problems by commits that consist exclusively of deletions?
PR kern/58091: after fork/execve or posix_spawn, parent kill(child,
SIGTERM) has race condition making it unreliable
To generate a diff of this commit:
cvs rdiff -u -r1.409 -r1.410 src/sys/kern/kern_sig.c
cvs rdiff -u -r1.3 -r1.4 src/tests/lib/libc/gen/execve/t_execve.c
cvs rdiff -u -r1.9 -r1.10 src/tests/lib/libc/gen/posix_spawn/t_spawn.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/kern_sig.c
diff -u src/sys/kern/kern_sig.c:1.409 src/sys/kern/kern_sig.c:1.410
--- src/sys/kern/kern_sig.c:1.409 Sat Feb 10 09:24:18 2024
+++ src/sys/kern/kern_sig.c Thu Mar 13 12:48:21 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_sig.c,v 1.409 2024/02/10 09:24:18 andvar Exp $ */
+/* $NetBSD: kern_sig.c,v 1.410 2025/03/13 12:48:21 riastradh Exp $ */
/*-
* Copyright (c) 2006, 2007, 2008, 2019, 2023 The NetBSD Foundation, Inc.
@@ -70,7 +70,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_sig.c,v 1.409 2024/02/10 09:24:18 andvar Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_sig.c,v 1.410 2025/03/13 12:48:21 riastradh Exp $");
#include "opt_execfmt.h"
#include "opt_ptrace.h"
@@ -123,7 +123,6 @@ sigset_t stopsigmask __cacheline_aligne
static sigset_t vforksigmask __cacheline_aligned;
sigset_t sigcantmask __cacheline_aligned;
-static void ksiginfo_exechook(struct proc *, void *);
static void proc_stop(struct proc *, int);
static void proc_stop_done(struct proc *, int);
static void proc_stop_callout(void *);
@@ -218,8 +217,6 @@ signal_init(void)
ksiginfo_cache = pool_cache_init(sizeof(ksiginfo_t), 0, 0, 0,
"ksiginfo", NULL, IPL_VM, NULL, NULL, NULL);
- exechook_establish(ksiginfo_exechook, NULL);
-
callout_init(&proc_stop_ch, CALLOUT_MPSAFE);
callout_setfunc(&proc_stop_ch, proc_stop_callout, NULL);
@@ -441,29 +438,6 @@ execsigs(struct proc *p)
}
/*
- * ksiginfo_exechook:
- *
- * Free all pending ksiginfo entries from a process on exec.
- * Additionally, drain any unused ksiginfo structures in the
- * system back to the pool.
- *
- * XXX This should not be a hook, every process has signals.
- */
-static void
-ksiginfo_exechook(struct proc *p, void *v)
-{
- ksiginfoq_t kq;
-
- ksiginfo_queue_init(&kq);
-
- mutex_enter(p->p_lock);
- sigclearall(p, NULL, &kq);
- mutex_exit(p->p_lock);
-
- ksiginfo_queue_drain(&kq);
-}
-
-/*
* ksiginfo_alloc:
*
* Allocate a new ksiginfo structure from the pool, and optionally copy
Index: src/tests/lib/libc/gen/execve/t_execve.c
diff -u src/tests/lib/libc/gen/execve/t_execve.c:1.3 src/tests/lib/libc/gen/execve/t_execve.c:1.4
--- src/tests/lib/libc/gen/execve/t_execve.c:1.3 Thu Mar 13 01:27:27 2025
+++ src/tests/lib/libc/gen/execve/t_execve.c Thu Mar 13 12:48:22 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: t_execve.c,v 1.3 2025/03/13 01:27:27 riastradh Exp $ */
+/* $NetBSD: t_execve.c,v 1.4 2025/03/13 12:48:22 riastradh Exp $ */
/*-
* Copyright (c) 2014 The NetBSD Foundation, Inc.
@@ -27,7 +27,7 @@
*/
#include <sys/cdefs.h>
-__RCSID("$NetBSD: t_execve.c,v 1.3 2025/03/13 01:27:27 riastradh Exp $");
+__RCSID("$NetBSD: t_execve.c,v 1.4 2025/03/13 12:48:22 riastradh Exp $");
#include <sys/wait.h>
@@ -74,10 +74,6 @@ ATF_TC_BODY(t_execve_sig, tc)
snprintf(h_execsig, sizeof(h_execsig), "%s/../h_execsig", srcdir);
REQUIRE_LIBC(signal(SIGPIPE, SIG_IGN), SIG_ERR);
- atf_tc_expect_fail("PR kern/580911: after fork/execve or posix_spawn,"
- " parent kill(child, SIGTERM) has race condition"
- " making it unreliable");
-
for (start = time(NULL); time(NULL) - start <= 10;) {
int fd[2];
char *const argv[] = {h_execsig, NULL};
Index: src/tests/lib/libc/gen/posix_spawn/t_spawn.c
diff -u src/tests/lib/libc/gen/posix_spawn/t_spawn.c:1.9 src/tests/lib/libc/gen/posix_spawn/t_spawn.c:1.10
--- src/tests/lib/libc/gen/posix_spawn/t_spawn.c:1.9 Thu Mar 13 01:27:27 2025
+++ src/tests/lib/libc/gen/posix_spawn/t_spawn.c Thu Mar 13 12:48:22 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: t_spawn.c,v 1.9 2025/03/13 01:27:27 riastradh Exp $ */
+/* $NetBSD: t_spawn.c,v 1.10 2025/03/13 12:48:22 riastradh Exp $ */
/*-
* Copyright (c) 2012, 2021 The NetBSD Foundation, Inc.
@@ -31,7 +31,7 @@
*/
#include <sys/cdefs.h>
-__RCSID("$NetBSD: t_spawn.c,v 1.9 2025/03/13 01:27:27 riastradh Exp $");
+__RCSID("$NetBSD: t_spawn.c,v 1.10 2025/03/13 12:48:22 riastradh Exp $");
#include <atf-c.h>
@@ -602,10 +602,6 @@ ATF_TC_BODY(t_spawn_sig, tc)
snprintf(h_execsig, sizeof(h_execsig), "%s/../h_execsig", srcdir);
REQUIRE_LIBC(signal(SIGPIPE, SIG_IGN), SIG_ERR);
- atf_tc_expect_fail("PR kern/580911: after fork/execve or posix_spawn,"
- " parent kill(child, SIGTERM) has race condition"
- " making it unreliable");
-
for (start = time(NULL); time(NULL) - start <= 10;) {
int fd[2];
char *const argv[] = {h_execsig, NULL};
