NetBSD Problem Report #57527
From mlelstv@serpens.de Sat Jul 15 15:17:00 2023
Return-Path: <mlelstv@serpens.de>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 543F81A923E
for <gnats-bugs@gnats.NetBSD.org>; Sat, 15 Jul 2023 15:17:00 +0000 (UTC)
Message-Id: <20230715145830.B47E8CCAE7@tazz.1st.de>
Date: Sat, 15 Jul 2023 16:58:30 +0200 (CEST)
From: mlelstv@netbsd.org
Reply-To: mlelstv@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: system() waits for last child if SIG_CHLD is ignored
X-Send-Pr-Version: 3.95
>Number: 57527
>Category: kern
>Synopsis: system() waits for last child if SIG_CHLD is ignored
>Confidential: no
>Severity: critical
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat Jul 15 15:20:00 +0000 2023
>Last-Modified: Sat Aug 12 12:55:01 +0000 2023
>Originator: Michael van Elst
>Release: NetBSD 10.99.5
>Organization:
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
>Environment:
System: NetBSD tazz 10.99.5 NetBSD 10.99.5 (TAZZ) #268: Sat Jul 15 14:39:30 UTC 2023 mlelstv@slowpoke:/scratch2/obj.amd64/scratch/netbsd-current/src/sys/arch/amd64/compile/TAZZ amd64
Architecture: x86_64
Machine: amd64
>Description:
When you ignore SIGCHLD to avoid zombies, then a call to system()
waits until the last child exited.
This is actually wait() or waitpid() waiting (which are called by system()).
>How-To-Repeat:
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
int main(int argc, char *argv[]) {
pid_t pid;
pid = fork();
if (pid == 0) {
sleep(4);
_exit(0);
}
signal(SIGCHLD, SIG_IGN);
system("sleep 1");
return 0;
}
Sleeps 4 seconds on NetBSD and 1 second on Linux.
Tis behaviour comes explicit from exit1() in kern_exit.c:
/*
* If this was the last child of our parent, notify
* parent, so in case he was wait(2)ing, he will
* continue.
*/
if (LIST_FIRST(&old_parent->p_children) == NULL)
cv_broadcast(&old_parent->p_waitcv);
FreeBSD doesn't have such a condition and always wakes the parent.
/*
* Notify parent, so in case he was wait(2)ing or
* executing waitpid(2) with our pid, he will
* continue.
*/
wakeup(pp);
Removing the condition in exit1() fixes the behaviour, but there might
be side effects.
>Fix:
>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: mlelstv@NetBSD.org
Cc: gnats-bugs@NetBSD.org
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>
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--
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/57527: system() waits for last child if SIG_CHLD is ignored
Date: Sat, 15 Jul 2023 20:46:43 +0000
On Sat, Jul 15, 2023 at 08:05:02PM +0000, Taylor R Campbell wrote:
> 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)?
It's worse than that; you could race with another thread setting
SIG_IGN.
Maybe that gets into "don't do that" territory though.
--
David A. Holland
dholland@netbsd.org
From: Taylor R Campbell <riastradh@NetBSD.org>
To: dholland@NetBSD.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/57527: system() waits for last child if SIG_CHLD is ignored
Date: Sat, 15 Jul 2023 21:48:47 +0000
This is a multi-part message in MIME format.
--=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV
> Date: Sat, 15 Jul 2023 20:46:43 +0000
> From: David Holland <dholland-bugs@netbsd.org>
>
> On Sat, Jul 15, 2023 at 08:05:02PM +0000, Taylor R Campbell wrote:
> > 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)?
>
> It's worse than that; you could race with another thread setting
> SIG_IGN.
>
> Maybe that gets into "don't do that" territory though.
Yes, it's in `don't do that' territory. POSIX says:
Using the system() function in more than one thread in a process or
when the SIGCHLD signal is being manipulated by more than one
thread in a process may produce unexpected results.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html
--=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV
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>
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
--=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV--
From: Taylor R Campbell <riastradh@NetBSD.org>
To: mlelstv@NetBSD.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/57527: system() waits for last child if SIG_CHLD is ignored
Date: Mon, 17 Jul 2023 12:45:07 +0000
This is a multi-part message in MIME format.
--=_skMWPwotJfxMxufC+Kk6N1dx2rTmwBMC
> Date: Sat, 15 Jul 2023 20:04:50 +0000
> From: Taylor R Campbell <riastradh@NetBSD.org>
>
> 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
Sorry, that's an ancient POSIX URL. Corrected URL that has the quote:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/wait.html
The attached three-patch series adds automatic tests for these corners
of system(3), and shows that the temporary SIG_IGN -> SIG_DFL change
addresses them.
However, it doesn't address the case of a signal handler function (not
SIG_IGN) with SA_NOCLDWAIT set, as demonstrated in the third patch.
That sounds difficult to reconcile:
- If we leave the signal handler in place, waitpid doesn't work.
- If we touch sigaction, queued signals are lost.
- If we try to requeue signals with raise or kill, siginfo is
destroyed.
What to do? There's some long text in POSIX about whether wait(2)
consumes (`accepts'/`discards') SIGCHLD signals and its interaction
with system(3), but I'm out of time for now to think further about
this.
--=_skMWPwotJfxMxufC+Kk6N1dx2rTmwBMC
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57527-system-v3"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57527-system-v3.patch"
From 83ffe8640761b46ca49538a438c741f7f24d4acb Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Mon, 17 Jul 2023 11:51:09 +0000
Subject: [PATCH 1/3] system(3): Add test for SA_NOCLDWAIT and SIG_IGN SIGCH=
LD
interaction.
PR 57527
---
tests/lib/libc/stdlib/Makefile | 1 +
tests/lib/libc/stdlib/t_system_sigchld.c | 459 +++++++++++++++++++++++
2 files changed, 460 insertions(+)
create mode 100644 tests/lib/libc/stdlib/t_system_sigchld.c
diff --git a/tests/lib/libc/stdlib/Makefile b/tests/lib/libc/stdlib/Makefile
index a7221b5033fc..493152d47882 100644
--- a/tests/lib/libc/stdlib/Makefile
+++ b/tests/lib/libc/stdlib/Makefile
@@ -20,6 +20,7 @@ TESTS_C+=3D t_strtod
TESTS_C+=3D t_strtol
TESTS_C+=3D t_strtoi
TESTS_C+=3D t_system
+TESTS_C+=3D t_system_sigchld
=20
TESTS_SH+=3D t_atexit
TESTS_SH+=3D t_getopt
diff --git a/tests/lib/libc/stdlib/t_system_sigchld.c b/tests/lib/libc/stdl=
ib/t_system_sigchld.c
new file mode 100644
index 000000000000..9463fc5994ec
--- /dev/null
+++ b/tests/lib/libc/stdlib/t_system_sigchld.c
@@ -0,0 +1,459 @@
+/* $NetBSD$ */
+
+/*-
+ * Copyright (c) 2023 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTO=
RS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIM=
ITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICU=
LAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTO=
RS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF =
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <sys/cdefs.h>
+__RCSID("$NetBSD: t_system.c,v 1.1 2011/09/11 10:32:23 jruoho Exp $");
+
+#include <sys/wait.h>
+
+#include <atf-c.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "h_macros.h"
+
+static void
+emptyhandler(int signo)
+{
+}
+
+static bool woken;
+
+static void
+wakeup(int signo)
+{
+
+ woken =3D true;
+}
+
+static bool sigchld;
+
+static void
+sigchldhandler(int signo)
+{
+
+ sigchld =3D true;
+}
+
+static siginfo_t sigchldinfo;
+
+static void
+sigchldaction(int signo, siginfo_t *info, void *ctx)
+{
+
+ sigchld =3D true;
+ sigchldinfo =3D *info;
+}
+
+static void
+go(bool dowait)
+{
+ int pfd[2], ffd;
+ char c =3D 1;
+ ssize_t n;
+ int status =3D 0;
+ pid_t child, waitret;
+
+ /*
+ * Create a pipe for communication with the asynchronous child
+ * process.
+ */
+ RL(pipe(pfd));
+
+ /*
+ * Fork an asynchronous child process that will wait until
+ * we've written to the pipe before exiting.
+ */
+ RL(child =3D fork());
+ if (child =3D=3D 0) {
+ if ((n =3D read(pfd[0], &c, 1)) =3D=3D -1)
+ _exit(1);
+ if (n !=3D 1)
+ _exit(1);
+ _exit(0);
+ }
+
+ /*
+ * Set an alarm to kill the process if system(3) hasn't
+ * returned in 5sec.
+ */
+ ATF_REQUIRE_MSG(signal(SIGALRM, &wakeup) !=3D SIG_ERR,
+ "signal(SIGALRM): %s", strerror(errno));
+ RL((int)alarm(2));
+
+ /*
+ * Run a synchronous child process. This should complete
+ * promptly without error. Paranoia: make sure the output file
+ * is not exist already.
+ */
+ (void)unlink("ok");
+ RL(system("touch ok"));
+
+ /*
+ * Clear the alarm. Fail if it had woken us already.
+ */
+ RL((int)alarm(0));
+ ATF_REQUIRE(!woken);
+
+ /*
+ * Make sure the synchronous child process took effect by
+ * creating a file `ok'.
+ */
+ RL(ffd =3D open("ok", O_RDONLY));
+ RL(close(ffd));
+
+ /*
+ * Wake the asynchronous child process so it can exit.
+ */
+ RL(n =3D write(pfd[1], &c, 1));
+ ATF_REQUIRE_MSG(n =3D=3D 1, "n=3D%zd", n);
+
+ /*
+ * Wait for the asynchronous child process -- or expect waitpid
+ * to fail with ECHILD if we can't. Set an alarm so the wait
+ * can't take too long.
+ */
+ RL((int)alarm(2));
+ waitret =3D waitpid(child, &status, 0);
+ if (dowait) {
+ ATF_REQUIRE_EQ_MSG(waitret, child,
+ "waitret=3D%ld status=3D0x%x child=3D%ld",
+ (long)waitret, status, (long)child);
+ ATF_REQUIRE(WIFEXITED(status));
+ ATF_REQUIRE_EQ_MSG(WEXITSTATUS(status), 0,
+ "status=3D0x%x WEXITSTATUS(status)=3D%d",
+ status, WEXITSTATUS(status));
+ } else {
+ ATF_REQUIRE_EQ_MSG(waitret, -1,
+ "waitret=3D%ld status=3D0x%x child=3D%ld",
+ (long)waitret, status, (long)child);
+ ATF_REQUIRE_EQ_MSG(errno, ECHILD, "errno=3D%d %s",
+ errno, strerror(errno));
+ }
+ RL((int)alarm(0));
+ ATF_REQUIRE(!woken);
+
+ RL(close(pfd[0]));
+ RL(close(pfd[1]));
+}
+
+static void
+goqueued(bool dowait)
+{
+ sigset_t block, omask;
+ pid_t child0, waitret0;
+ int status0 =3D 0;
+
+ /*
+ * Block SIGCHLD for most of the test -- we are going to make
+ * sure that the signal remains queued.
+ */
+ RL(sigemptyset(&block));
+ RL(sigaddset(&block, SIGCHLD));
+ RL(sigprocmask(SIG_BLOCK, &block, &omask));
+
+ /*
+ * Fork a child that immediately exits -- this should promptly
+ * deliver a queued SIGCHLD.
+ */
+ RL(child0 =3D fork());
+ if (child0 =3D=3D 0)
+ _exit(0);
+
+ /*
+ * Wait for the child and verify it exited cleanly. This
+ * should ensure the SIGCHLD is queued before we proceed.
+ */
+ waitret0 =3D waitpid(child0, &status0, 0);
+ if (dowait) {
+ ATF_REQUIRE_EQ_MSG(waitret0, child0,
+ "waitret0=3D%ld status0=3D0x%x child0=3D%ld",
+ (long)waitret0, status0, (long)child0);
+ ATF_REQUIRE(WIFEXITED(status0));
+ ATF_REQUIRE_EQ_MSG(WEXITSTATUS(status0), 0,
+ "status0=3D0x%x WEXITSTATUS(status0)=3D%d",
+ status0, WEXITSTATUS(status0));
+ } else {
+ ATF_REQUIRE_EQ_MSG(waitret0, -1,
+ "waitret0=3D%ld status0=3D0x%x child0=3D%ld",
+ (long)waitret0, status0, (long)child0);
+ ATF_REQUIRE_EQ_MSG(errno, ECHILD, "errno=3D%d %s",
+ errno, strerror(errno));
+ }
+
+ /*
+ * Do the main test.
+ */
+ go(dowait);
+
+ /*
+ * Unblock SIGCHLD and verify that we got the signal.
+ */
+ RL(sigprocmask(SIG_SETMASK, &omask, NULL));
+ ATF_REQUIRE(sigchld);
+}
+
+ATF_TC(system_default);
+ATF_TC_HEAD(system_default, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "system(3) with asynchronous child");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_default, tc)
+{
+ go(/*dowait*/true);
+}
+
+ATF_TC(system_nocldwait);
+ATF_TC_HEAD(system_nocldwait, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child using SA_NOCLDWAIT");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_nocldwait, tc)
+{
+ struct sigaction sa =3D {
+ .sa_handler =3D &emptyhandler,
+ .sa_flags =3D SA_NOCLDWAIT,
+ };
+ struct sigaction nsa;
+
+ atf_tc_expect_fail("PR 57527");
+
+ RL(sigaction(SIGCHLD, &sa, NULL));
+
+ /*
+ * With SIGCHLD set with SA_NOCLDWAIT, the asynchronous child
+ * process will be reaped without waiting.
+ */
+ go(/*dowait*/false);
+
+ /*
+ * Verify the signal configuration hasn't changed.
+ */
+ RL(sigaction(SIGCHLD, NULL, &nsa));
+ ATF_REQUIRE_EQ_MSG(nsa.sa_handler, &emptyhandler, "%p",
+ nsa.sa_handler);
+ ATF_REQUIRE_EQ_MSG(nsa.sa_flags, SA_NOCLDWAIT, "0x%x", nsa.sa_flags);
+}
+
+ATF_TC(system_sigign);
+ATF_TC_HEAD(system_sigign, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child using SIG_IGN");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_sigign, tc)
+{
+ void (*h)(int);
+
+ atf_tc_expect_fail("PR 57527");
+
+ ATF_REQUIRE_MSG(signal(SIGCHLD, SIG_IGN) !=3D SIG_ERR,
+ "signal(SIGCHLD): %s", strerror(errno));
+
+ /*
+ * With SIGCHLD set to SIG_IGN, the asynchronous child process
+ * will be reaped without waiting.
+ */
+ go(/*dowait*/false);
+
+ /*
+ * Verify the signal configuration hasn't changed.
+ */
+ h =3D signal(SIGCHLD, SIG_DFL);
+ ATF_REQUIRE_EQ_MSG(h, SIG_IGN, "%p", h);
+}
+
+ATF_TC(system_sigign_nocldwait);
+ATF_TC_HEAD(system_sigign_nocldwait, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child"
+ " using SIG_IGN and SA_NOCLDWAIT");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_sigign_nocldwait, tc)
+{
+ struct sigaction sa =3D {
+ .sa_handler =3D SIG_IGN,
+ .sa_flags =3D SA_NOCLDWAIT,
+ };
+ struct sigaction nsa;
+
+ atf_tc_expect_fail("PR 57527");
+
+ RL(sigaction(SIGCHLD, &sa, NULL));
+
+ /*
+ * With SIGCHLD set to SIG_IGN and/or SA_NOCLDWAIT set, the
+ * asynchronous child process will be reaped without waiting.
+ */
+ go(/*dowait*/false);
+
+ RL(sigaction(SIGCHLD, NULL, &nsa));
+ ATF_REQUIRE_EQ_MSG(nsa.sa_handler, SIG_IGN, "%p", nsa.sa_handler);
+ ATF_REQUIRE_EQ_MSG(nsa.sa_flags, SA_NOCLDWAIT, "0x%x", nsa.sa_flags);
+}
+
+ATF_TC(system_queued);
+ATF_TC_HEAD(system_queued, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child and a queued SIGCHLD");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_queued, tc)
+{
+ void (*h)(int);
+
+ /*
+ * Set up a signal handler to set a flag when SIGCHLD is
+ * delivered.
+ */
+ ATF_REQUIRE_MSG(signal(SIGCHLD, &sigchldhandler) !=3D SIG_ERR,
+ "signal(SIGCHLD): %s", strerror(errno));
+
+ goqueued(/*dowait*/true);
+
+ /*
+ * Verify the signal configuration hasn't changed.
+ */
+ h =3D signal(SIGCHLD, SIG_DFL);
+ ATF_REQUIRE_EQ_MSG(h, &sigchldhandler, "%p", h);
+}
+
+ATF_TC(system_queued_action);
+ATF_TC_HEAD(system_queued_action, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child and a queued SIGCHLD,"
+ " with sigaction");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_queued_action, tc)
+{
+ struct sigaction sa =3D {
+ .sa_sigaction =3D &sigchldaction,
+ .sa_flags =3D SA_SIGINFO,
+ };
+ struct sigaction nsa;
+
+ /*
+ * Set up a signal action to set a flag when SIGCHLD is
+ * delivered.
+ */
+ RL(sigaction(SIGCHLD, &sa, NULL));
+
+ goqueued(/*dowait*/true);
+
+ /*
+ * Verify we got the right siginfo.
+ */
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_signo, SIGCHLD, "%d",
+ sigchldinfo.si_signo);
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_errno, 0, "%d",
+ sigchldinfo.si_errno);
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_code, CLD_EXITED, "%d",
+ sigchldinfo.si_code);
+
+ /*
+ * Verify the signal configuration hasn't changed.
+ */
+ RL(sigaction(SIGCHLD, NULL, &nsa));
+ ATF_REQUIRE_EQ_MSG(nsa.sa_sigaction, &sigchldaction,
+ "%p", nsa.sa_sigaction);
+ ATF_REQUIRE_EQ_MSG(nsa.sa_flags, SA_SIGINFO,
+ "0x%x", nsa.sa_flags);
+}
+
+ATF_TC(system_queued_nocldwait);
+ATF_TC_HEAD(system_queued_nocldwait, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "system(3) with asynchronous child and a queued SIGCHLD,"
+ " with SA_NOCLDWAIT");
+ atf_tc_set_md_var(tc, "require.progs", "touch");
+}
+ATF_TC_BODY(system_queued_nocldwait, tc)
+{
+ struct sigaction sa =3D {
+ .sa_sigaction =3D &sigchldaction,
+ .sa_flags =3D SA_NOCLDWAIT|SA_SIGINFO,
+ };
+ struct sigaction nsa;
+
+ atf_tc_expect_fail("PR 57527");
+
+ /*
+ * Set up a signal action to set a flag when SIGCHLD is
+ * delivered.
+ */
+ RL(sigaction(SIGCHLD, &sa, NULL));
+
+ goqueued(/*dowait*/false);
+
+ /*
+ * Verify we got the right siginfo.
+ */
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_signo, SIGCHLD, "%d",
+ sigchldinfo.si_signo);
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_errno, 0, "%d",
+ sigchldinfo.si_errno);
+ ATF_CHECK_EQ_MSG(sigchldinfo.si_code, CLD_EXITED, "%d",
+ sigchldinfo.si_code);
+
+ /*
+ * Verify the signal configuration hasn't changed.
+ */
+ RL(sigaction(SIGCHLD, NULL, &nsa));
+ ATF_REQUIRE_EQ_MSG(nsa.sa_sigaction, &sigchldaction,
+ "%p", nsa.sa_sigaction);
+ ATF_REQUIRE_EQ_MSG(nsa.sa_flags, SA_NOCLDWAIT|SA_SIGINFO,
+ "0x%x", nsa.sa_flags);
+}
+
+ATF_TP_ADD_TCS(tp)
+{
+
+ ATF_TP_ADD_TC(tp, system_default);
+ ATF_TP_ADD_TC(tp, system_nocldwait);
+ ATF_TP_ADD_TC(tp, system_queued);
+ ATF_TP_ADD_TC(tp, system_queued_action);
+ ATF_TP_ADD_TC(tp, system_queued_nocldwait);
+ ATF_TP_ADD_TC(tp, system_sigign);
+ ATF_TP_ADD_TC(tp, system_sigign_nocldwait);
+
+ return atf_no_error();
+}
From daf08fb3988bd6de0ec12348c12a6131ddc279e5 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 15 Jul 2023 20:02:28 +0000
Subject: [PATCH 2/3] system(3): Change SIG_IGN to SIG_DFL for SIGCHLD while=
we
work.
Note: This doesn't address the scenario that SIGCHLD is set to a
handler function, not SIG_IGN, but has the SA_NOCLDWAIT flag set.
PR 57527
---
lib/libc/stdlib/system.c | 42 ++++++++++++++++++++++--
tests/lib/libc/stdlib/t_system_sigchld.c | 4 ---
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/lib/libc/stdlib/system.c b/lib/libc/stdlib/system.c
index 5a19bc9e0568..8101278c1bd3 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,42 @@ 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 in wait/waitpid:
+ *
+ * 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/9699919799/functions/wait.html
+ *
+ * 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 +169,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
diff --git a/tests/lib/libc/stdlib/t_system_sigchld.c b/tests/lib/libc/stdl=
ib/t_system_sigchld.c
index 9463fc5994ec..78d44a8bd797 100644
--- a/tests/lib/libc/stdlib/t_system_sigchld.c
+++ b/tests/lib/libc/stdlib/t_system_sigchld.c
@@ -278,8 +278,6 @@ ATF_TC_BODY(system_sigign, tc)
{
void (*h)(int);
=20
- atf_tc_expect_fail("PR 57527");
-
ATF_REQUIRE_MSG(signal(SIGCHLD, SIG_IGN) !=3D SIG_ERR,
"signal(SIGCHLD): %s", strerror(errno));
=20
@@ -312,8 +310,6 @@ ATF_TC_BODY(system_sigign_nocldwait, tc)
};
struct sigaction nsa;
=20
- atf_tc_expect_fail("PR 57527");
-
RL(sigaction(SIGCHLD, &sa, NULL));
=20
/*
From 57a2d906999c44e409cdf3bc50d84615b390a0c0 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Mon, 17 Jul 2023 12:37:06 +0000
Subject: [PATCH 3/3] WIP: system(3): defend against SA_NOCLDWAIT too
---
lib/libc/stdlib/system.c | 4 ++--
tests/lib/libc/stdlib/t_system_sigchld.c | 2 --
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/libc/stdlib/system.c b/lib/libc/stdlib/system.c
index 8101278c1bd3..256324a0869e 100644
--- a/lib/libc/stdlib/system.c
+++ b/lib/libc/stdlib/system.c
@@ -117,7 +117,7 @@ system(const char *command)
sigaction(SIGQUIT, &quitsa, NULL);
return -1;
}
- if (chldsa.sa_handler =3D=3D SIG_IGN &&
+ if ((chldsa.sa_handler =3D=3D SIG_IGN || chldsa.sa_flags & SA_NOCLDWAIT) =
&&
sigaction(SIGCHLD, &(struct sigaction){ .sa_handler =3D SIG_DFL },
NULL) =3D=3D -1) {
(void)sigprocmask(SIG_SETMASK, &omask, NULL);
@@ -169,7 +169,7 @@ system(const char *command)
}
}
=20
-out: if (chldsa.sa_handler =3D=3D SIG_IGN)
+out: if (chldsa.sa_handler =3D=3D SIG_IGN || chldsa.sa_flags & SA_NOCLDWAI=
T)
sigaction(SIGCHLD, &chldsa, NULL);
sigaction(SIGINT, &intsa, NULL);
sigaction(SIGQUIT, &quitsa, NULL);
diff --git a/tests/lib/libc/stdlib/t_system_sigchld.c b/tests/lib/libc/stdl=
ib/t_system_sigchld.c
index 78d44a8bd797..88ad880efef2 100644
--- a/tests/lib/libc/stdlib/t_system_sigchld.c
+++ b/tests/lib/libc/stdlib/t_system_sigchld.c
@@ -248,8 +248,6 @@ ATF_TC_BODY(system_nocldwait, tc)
};
struct sigaction nsa;
=20
- atf_tc_expect_fail("PR 57527");
-
RL(sigaction(SIGCHLD, &sa, NULL));
=20
/*
--=_skMWPwotJfxMxufC+Kk6N1dx2rTmwBMC--
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57527 CVS commit: src/share/man/man7
Date: Mon, 17 Jul 2023 14:20:19 +0000
Module Name: src
Committed By: riastradh
Date: Mon Jul 17 14:20:19 UTC 2023
Modified Files:
src/share/man/man7: signal.7
Log Message:
signal(7): Clarify semantics of SIGCHLD with SIG_IGN or SA_NOCLDWAIT.
The semantics is not just a nonportable hack for SysV compatibility;
it is enshrined in POSIX.
Related: PR 57527
To generate a diff of this commit:
cvs rdiff -u -r1.27 -r1.28 src/share/man/man7/signal.7
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57527 CVS commit: [netbsd-10] src/share/man/man7
Date: Sat, 12 Aug 2023 12:53:18 +0000
Module Name: src
Committed By: martin
Date: Sat Aug 12 12:53:18 UTC 2023
Modified Files:
src/share/man/man7 [netbsd-10]: signal.7
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #325):
share/man/man7/signal.7: revision 1.27
share/man/man7/signal.7: revision 1.28
signal(7): Clarify quirky SysV-inspired SIGCHLD semantics.
Suggest a portable alternative approach for detaching subprocesses.
Break wall of text into paragraphs while here.
signal(7): Clarify semantics of SIGCHLD with SIG_IGN or SA_NOCLDWAIT.
The semantics is not just a nonportable hack for SysV compatibility;
it is enshrined in POSIX.
Related: PR 57527
To generate a diff of this commit:
cvs rdiff -u -r1.26 -r1.26.4.1 src/share/man/man7/signal.7
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2023
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home