CVS commit: src

["Taylor R Campbell" <riastradh%netbsd.org@localhost>]

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.