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" <[email protected]>
To: [email protected]
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.