NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
PR/58091 CVS commit: [netbsd-10] src
The following reply was made to PR kern/58091; it has been noted by GNATS.
From: "Martin Husemann" <martin%netbsd.org@localhost>
To: gnats-bugs%gnats.NetBSD.org@localhost
Cc:
Subject: PR/58091 CVS commit: [netbsd-10] src
Date: Sat, 27 Jun 2026 17:08:21 +0000
Module Name: src
Committed By: martin
Date: Sat Jun 27 17:08:21 UTC 2026
Modified Files:
src/distrib/sets/lists/debug [netbsd-10]: mi
src/distrib/sets/lists/tests [netbsd-10]: mi
src/sys/kern [netbsd-10]: kern_sig.c
src/tests/lib/libc/gen [netbsd-10]: Makefile
src/tests/lib/libc/gen/execve [netbsd-10]: t_execve.c
src/tests/lib/libc/gen/posix_spawn [netbsd-10]: t_spawn.c
Added Files:
src/tests/lib/libc/gen [netbsd-10]: Makefile.inc h_execsig.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #1282):
tests/lib/libc/gen/posix_spawn/t_spawn.c: revision 1.9
tests/lib/libc/gen/execve/t_execve.c: revision 1.3
tests/lib/libc/gen/execve/t_execve.c: revision 1.4
sys/kern/kern_sig.c: revision 1.410
tests/lib/libc/gen/Makefile.inc: revision 1.1
distrib/sets/lists/tests/mi: revision 1.1361
distrib/sets/lists/debug/mi: revision 1.469
tests/lib/libc/gen/h_execsig.c: revision 1.1
tests/lib/libc/gen/posix_spawn/t_spawn.c: revision 1.10
tests/lib/libc/gen/posix_spawn/t_spawn.c: revision 1.11
tests/lib/libc/gen/Makefile: revision 1.57
execve(2), posix_spawn(2): Add test case for an embarrassing bug.
PR kern/58091: after fork/execve or posix_spawn, parent kill(child,
SIGTERM) has race condition making it unreliable
execve(2), posix_spawn(2): Fix build of tests after previous.
Forgot to add a file.
PR kern/58091: after fork/execve or posix_spawn, parent kill(child,
SIGTERM) has race condition making it unreliable
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
t_spawn: Add missing dup2 in t_spawn_sig.
Matches what h_execsig expects, and what t_execve_sig arranges: stdin
is a pipe that the parent will write a single byte to after it has
delivered SIGTERM.
Now this should have a higher chance of provoking the bug (though it
was already good enough in cursory testing!).
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.394.2.16 -r1.394.2.17 src/distrib/sets/lists/debug/mi
cvs rdiff -u -r1.1238.2.20 -r1.1238.2.21 src/distrib/sets/lists/tests/mi
cvs rdiff -u -r1.404 -r1.404.4.1 src/sys/kern/kern_sig.c
cvs rdiff -u -r1.55.2.1 -r1.55.2.2 src/tests/lib/libc/gen/Makefile
cvs rdiff -u -r0 -r1.1.6.2 src/tests/lib/libc/gen/Makefile.inc \
src/tests/lib/libc/gen/h_execsig.c
cvs rdiff -u -r1.2 -r1.2.26.1 src/tests/lib/libc/gen/execve/t_execve.c
cvs rdiff -u -r1.8 -r1.8.2.1 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.
Home |
Main Index |
Thread Index |
Old Index