PR/58091 CVS commit: src

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

The following reply was made to PR kern/58091; it has been noted by GNATS.

From: "Taylor R Campbell" <riastradh%netbsd.org@localhost>
To: gnats-bugs%gnats.NetBSD.org@localhost
Cc: 
Subject: PR/58091 CVS commit: src
Date: Thu, 13 Mar 2025 12:48:22 +0000

 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.