Re: kern/57527: system() waits for last child if SIG_CHLD is ignored

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

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

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: mlelstv%NetBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/57527: system() waits for last child if SIG_CHLD is ignored
Date: Sat, 15 Jul 2023 20:04:50 +0000

 This is a multi-part message in MIME format.
 --=_jTTFFMu/al3E0ztP4Shf9axJ0hgFpHTk
 
 The following clause in POSIX on wait/waitpid that mlelstv found
 shortly after filing the bug looks relevant:
 
    If the calling process has SA_NOCLDWAIT set or has SIGCHLD set to
    SIG_IGN, and the process has no unwaited for children that were
    transformed into zombie processes, the calling thread will block
    until all of the children of the process containing the calling
    thread terminate, and wait() and waitpid() will fail and set errno
    to [ECHILD].
 
    https://pubs.opengroup.org/onlinepubs/7908799/xsh/wait.html
 
 That is, this peculiar behaviour of waitpid that depends on whether
 SIGCHLD is set to SIG_IGN or has SA_NOCLDWAIT set is enshrined in
 POSIX.  So the kernel isn't doing something wrong, except...
 
 > Removing the condition in exit1() fixes the behaviour, but there
 > might be side effects.
 
 If issuing more _wakeups_ changes anything substantively, the
 _waiting_ side must be broken.  I think we're missing a test for
 PK_NOCLDWAIT|PK_CHLDSIGIGN in do_sys_waitid to match the conditional
 wakeup for this semantics, if we want to follow this semantics
 properly.  (What we're doing right now is wrong either way!)
 
 FreeBSD, however, appears to have intentionally eliminated this quirky
 behaviour of POSIX, in svn revision 191313:
 
    r191313 | kib | 2009-04-20 14:34:55 +0000 (Mon, 20 Apr 2009) | 14 lines
    On the exit of the child process which parent either set SA_NOCLDWAIT
    or ignored SIGCHLD, unconditionally wake up the parent instead of doing
    this only when the child is a last child.
    This brings us in line with other U**xes that support SA_NOCLDWAIT. If
    the parent called waitpid(childpid), then exit of the child should wake
    up the parent immediately instead of forcing it to wait for all children
    to exit.
 
                    /*
    -                * If this was the last child of our parent, notify
    -                * parent, so in case he was wait(2)ing, he will
    +                * Notify parent, so in case he was wait(2)ing or
    +                * executiing waitpid(2) with our pid, he will
                     * continue.
                     */
    -               if (LIST_EMPTY(&pp->p_children))
    -                       wakeup(pp);
    +               wakeup(pp);
 
 
 But let's suppose we intend to follow POSIX here.
 
 Under this semantics, it is tempting to say that system(3) should set
 SIGCHLD to SIG_DFL while it is blocked, in case the caller had set it
 to SIG_IGN.  Unfortunately, this has the side effect that if there
 were any queued SIGCHLD signals for processes _other_ than the one
 that system(3) spawned, those queued SIGCHLD signals will be lost and
 not delivered!  And we can't just raise(SIGCHLD) because that passes
 the wrong siginfo through.
 
 Maybe we could set SIGCHLD to SIG_DFL _only if_ it was already set to
 SIG_IGN, as in this note that mlelstv found:
 
 https://www.open-std.org/jtc1/sc22/WG15/docs/rr/9945-2/9945-2-28.html
 
 The rationale section of the wait/waitpid page explains some of the
 complications in interaction with system(3):
 
 https://pubs.opengroup.org/onlinepubs/7908799/xsh/wait.html
 
 However, it doesn't seem to address the possibility of changing
 SIGCHLD to SIG_DFL if and only if it was already set to SIG_IGN, if
 I'm reading it correctly.  So maybe the attached patch will resolve
 the issue in system(3)?
 
 --=_jTTFFMu/al3E0ztP4Shf9axJ0hgFpHTk
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57527-system"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57527-system.patch"
 
 From 51954a3b812c57c57835be1b4b06c5c9918c5783 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Sat, 15 Jul 2023 20:02:28 +0000
 Subject: [PATCH] system(3): Change SIG_IGN to SIG_DFL for SIGCHLD while we
  work.
 
 PR kern/57527
 ---
  lib/libc/stdlib/system.c | 32 ++++++++++++++++++++++++++++++--
  1 file changed, 30 insertions(+), 2 deletions(-)
 
 diff --git a/lib/libc/stdlib/system.c b/lib/libc/stdlib/system.c
 index 5a19bc9e0568..4d3e45f1892a 100644
 --- a/lib/libc/stdlib/system.c
 +++ b/lib/libc/stdlib/system.c
 @@ -54,7 +54,7 @@ int
  system(const char *command)
  {
  	pid_t pid;
 -	struct sigaction intsa, quitsa, sa;
 +	struct sigaction chldsa, intsa, quitsa, sa;
  	sigset_t nmask, omask, sigdefault;
  	int pstat;
  	const char *argp[] =3D {"sh", "-c", "--", command, NULL};
 @@ -90,6 +90,32 @@ system(const char *command)
  		return -1;
  	}
 =20
 +	/*
 +	 * Make sure SIGCHLD is not set to SIG_IGN, because that means
 +	 * waitpid will wait until _all_ children have exited, under a
 +	 * peculiar clause of POSIX enshrining a quirk of SysV
 +	 * semantics.
 +	 *
 +	 * If SIGCHLD was set to anything other than SIG_IGN, we must
 +	 * not interfere with it -- changing the action may clear any
 +	 * queued signals for for processes _other_ than the one that
 +	 * system(3) itself spawns.
 +	 */
 +	if (sigaction(SIGCHLD, NULL, &chldsa) =3D=3D -1) {
 +		(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 +		sigaction(SIGINT, &intsa, NULL);
 +		sigaction(SIGQUIT, &quitsa, NULL);
 +		return -1;
 +	}
 +	if (chldsa.sa_handler =3D=3D SIG_IGN &&
 +	    sigaction(SIGCHLD, &(struct sigaction){ .sa_handler =3D SIG_DFL },
 +		NULL) =3D=3D -1) {
 +		(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 +		sigaction(SIGINT, &intsa, NULL);
 +		sigaction(SIGQUIT, &quitsa, NULL);
 +		return -1;
 +	}
 +
  	/*
  	 * We arrange to inherit all signal handlers from the caller by
  	 * default, except possibly SIGINT and SIGQUIT.  These we have
 @@ -133,7 +159,9 @@ system(const char *command)
  		}
  	}
 =20
 -out:	sigaction(SIGINT, &intsa, NULL);
 +out:	if (chldsa.sa_handler =3D=3D SIG_IGN)
 +		sigaction(SIGCHLD, &chldsa, NULL);
 +	sigaction(SIGINT, &intsa, NULL);
  	sigaction(SIGQUIT, &quitsa, NULL);
  	(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 =20
 
 --=_jTTFFMu/al3E0ztP4Shf9axJ0hgFpHTk--