sd-event: Clean up SIGCHLD conditions for sd_event_add_child()

First, don't require blocking SIGCHLD for WEXITED. We watch for WEXITED
via pidfd instead of signalfd, so no need to insist on blocking SIGCHLD
anymore if we're only watching for WEXITED.

Second, do a proper check to see if the kernel autoreaping logic is
enabled. That has nothing to do with SIGCHLD being blocked for the current
thread or not. Instead, the kernel autoreaping logic is enabled either if
the disposition is set to SIG_IGN or if the SA_NOCLDWAIT flag is enabled.

diff --git a/TODO b/TODO
index d849c75a5fbd639567a63836074259bed6a95c6a..0d491158b84c29a15153e1e6f3876cb79bab9aa5 100644 (file)
--- a/TODO
+++ b/TODO
@@ -576,10 +576,6 @@ Features:
 * The bind(AF_UNSPEC) construct (for resetting sockets to their initial state)
   should be blocked in many cases because it punches holes in many sandboxes.
 
-* find a nice way to opt-in into auto-masking SIGCHLD on first
-  sd_event_add_child(), and then get rid of many more explicit sigprocmask()
-  calls.
-
 * introduce new structure Tpm2CombinedPolicy, that combines the various TPm2
   policy bits into one structure, i.e. public key info, pcr masks, pcrlock
   stuff, pin and so on. Then pass that around in tpm2_seal() and tpm2_unseal().

diff --git a/man/event-quick-child.c b/man/event-quick-child.c
index 828f0cd6f4b5737f2e1ffd22860cb22d8e6c7a11..a1d476243efe0fbabe853a9e64443407492676d3 100644 (file)
--- a/man/event-quick-child.c
+++ b/man/event-quick-child.c
@@ -10,12 +10,6 @@ int main(int argc, char **argv) {
   pid_t pid = fork();
   assert(pid >= 0);
 
-  /* SIGCHLD signal must be blocked for sd_event_add_child to work */
-  sigset_t ss;
-  sigemptyset(&ss);
-  sigaddset(&ss, SIGCHLD);
-  sigprocmask(SIG_BLOCK, &ss, NULL);
-
   if (pid == 0)  /* child */
     sleep(1);
 

diff --git a/man/sd_event_add_child.xml b/man/sd_event_add_child.xml
index 28ff6245165cb119890861b5ca4ee67cd965ea15..b0be46f03eac5098a2eb7501d0aa5e337405ae01 100644 (file)
--- a/man/sd_event_add_child.xml
+++ b/man/sd_event_add_child.xml
@@ -149,11 +149,13 @@
     <constant>SD_EVENT_OFF</constant> with
     <citerefentry><refentrytitle>sd_event_source_set_enabled</refentrytitle><manvolnum>3</manvolnum></citerefentry>.</para>
 
-    <para>The <constant>SIGCHLD</constant> signal must be blocked in all threads before this function is
-    called (using <citerefentry
-    project='man-pages'><refentrytitle>sigprocmask</refentrytitle><manvolnum>2</manvolnum></citerefentry> or
-    <citerefentry
-    project='man-pages'><refentrytitle>pthread_sigmask</refentrytitle><manvolnum>3</manvolnum></citerefentry>).</para>
+    <para>The kernel autoreaping logic must be disabled for this function to work as expected (see
+    <citerefentry project='man-pages'><refentrytitle>signal</refentrytitle><manvolnum>7</manvolnum></citerefentry>).</para>
+
+    <para>When watching for <constant>WSTOPPED</constant> or <constant>WCONTINUED</constant>, the
+    <constant>SIGCHLD</constant> signal must be blocked in all threads before this function is called (using
+    <citerefentry project='man-pages'><refentrytitle>sigprocmask</refentrytitle><manvolnum>2</manvolnum></citerefentry> or
+    <citerefentry project='man-pages'><refentrytitle>pthread_sigmask</refentrytitle><manvolnum>3</manvolnum></citerefentry>).</para>
 
     <para>If the second parameter of <function>sd_event_add_child()</function> is passed as
     <constant>NULL</constant> no reference to the event source object is returned. In this case, the event
@@ -190,7 +192,9 @@
     regardless which of <function>sd_event_add_child()</function> and
     <function>sd_event_add_child_pidfd()</function> is used for allocating an event source, the watched
     process has to be a direct child process of the invoking process. Also in both cases
-    <constant>SIGCHLD</constant> has to be blocked in the invoking process.</para>
+    <constant>SIGCHLD</constant> has to be blocked in the invoking process when watching for
+    <constant>WSTOPPED</constant> or <constant>WCONTINUED</constant> and the kernel autoreaping logic has to
+    be disabled.</para>
 
     <para><function>sd_event_source_get_child_pid()</function>
     retrieves the configured PID of a child process state change event

diff --git a/src/basic/signal-util.c b/src/basic/signal-util.c
index 2e5f7f24494f638a7990f932d03bcfda5cf7fdbc..a157462b9452c5aa5bb2c1e7c387a0d122692fb8 100644 (file)
--- a/src/basic/signal-util.c
+++ b/src/basic/signal-util.c
@@ -238,6 +238,15 @@ int signal_is_blocked(int sig) {
         return RET_NERRNO(sigismember(&ss, sig));
 }
 
+int autoreaping_enabled(void) {
+        struct sigaction sa;
+
+        if (sigaction(SIGCHLD, /* __act= */ NULL, &sa) < 0)
+                return -errno;
+
+        return sa.sa_handler == SIG_IGN || FLAGS_SET(sa.sa_flags, SA_NOCLDWAIT);
+}
+
 int pop_pending_signal_internal(int sig, ...) {
         sigset_t ss;
         va_list ap;

diff --git a/src/basic/signal-util.h b/src/basic/signal-util.h
index 78c4ae4c5692bfe8b70dc85e05d13a2494644413..298bd409a0d456d14e20afc807d5b73f6e8377a6 100644 (file)
--- a/src/basic/signal-util.h
+++ b/src/basic/signal-util.h
@@ -66,6 +66,8 @@ static inline const char* signal_to_string_with_check(int n) {
 
 int signal_is_blocked(int sig);
 
+int autoreaping_enabled(void);
+
 int pop_pending_signal_internal(int sig, ...);
 #define pop_pending_signal(...) pop_pending_signal_internal(__VA_ARGS__, -1)
 

diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c
index 5a9c54d8e1dc39040161f6e6fab638300323ca06..8c549eb22e0057873c62ef8acb1b853cef739683 100644 (file)
--- a/src/libsystemd/sd-event/sd-event.c
+++ b/src/libsystemd/sd-event/sd-event.c
@@ -1570,6 +1570,33 @@ static int child_exit_callback(sd_event_source *s, const siginfo_t *si, void *us
         return sd_event_exit(sd_event_source_get_event(s), PTR_TO_INT(userdata));
 }
 
+static int verify_sigchld(int options) {
+        int r;
+
+        if ((options & (WSTOPPED|WCONTINUED)) != 0) {
+                /* Caller must block SIGCHLD before using us to watch for WSTOPPED or WCONTINUED. */
+
+                r = signal_is_blocked(SIGCHLD);
+                if (r < 0)
+                        return r;
+                if (r == 0)
+                        return -EBUSY;
+        }
+
+        /* We don't want the Linux autoreaping logic to take effect when we're watching for process exit, so
+         * check if it is enabled. */
+
+        if (options & WEXITED) {
+                r = autoreaping_enabled();
+                if (r < 0)
+                        return r;
+                if (r > 0)
+                        return -EBUSY;
+        }
+
+        return 0;
+}
+
 _public_ int sd_event_add_child(
                 sd_event *e,
                 sd_event_source **ret,
@@ -1592,18 +1619,11 @@ _public_ int sd_event_add_child(
         if (!callback)
                 callback = child_exit_callback;
 
+        /* As an optimization we only do these checks on the first child event source created. */
         if (e->n_online_child_sources == 0) {
-                /* Caller must block SIGCHLD before using us to watch children, even if pidfd is available,
-                 * for compatibility with pre-pidfd and because we don't want the reap the child processes
-                 * ourselves, i.e. call waitid(), and don't want Linux' default internal logic for that to
-                 * take effect.
-                 *
-                 * (As an optimization we only do this check on the first child event source created.) */
-                r = signal_is_blocked(SIGCHLD);
+                r = verify_sigchld(options);
                 if (r < 0)
                         return r;
-                if (r == 0)
-                        return -EBUSY;
         }
 
         r = hashmap_ensure_allocated(&e->child_sources, NULL);
@@ -1617,8 +1637,8 @@ _public_ int sd_event_add_child(
         if (!s)
                 return -ENOMEM;
 
-        /* We always take a pidfd here if we can, even if we wait for anything else than WEXITED, so that we
-         * pin the PID, and make regular waitid() handling race-free. */
+        /* We always take a pidfd here, even if we wait for anything else than WEXITED, so that we pin the
+         * PID, and make regular waitid() handling race-free. */
 
         s->child.pidfd = pidfd_open(pid, 0);
         if (s->child.pidfd < 0)
@@ -1685,11 +1705,9 @@ _public_ int sd_event_add_child_pidfd(
                 callback = child_exit_callback;
 
         if (e->n_online_child_sources == 0) {
-                r = signal_is_blocked(SIGCHLD);
+                r = verify_sigchld(options);
                 if (r < 0)
                         return r;
-                if (r == 0)
-                        return -EBUSY;
         }
 
         r = hashmap_ensure_allocated(&e->child_sources, NULL);

diff --git a/src/libsystemd/sd-event/test-event.c b/src/libsystemd/sd-event/test-event.c
index 16bbd1296b139547cd3df2eee7005e128f9181f8..648daf7c81d5b6b7ba6535baf49b580185cde1b8 100644 (file)
--- a/src/libsystemd/sd-event/test-event.c
+++ b/src/libsystemd/sd-event/test-event.c
@@ -6,12 +6,14 @@
 #include "sd-event.h"
 
 #include "alloc-util.h"
+#include "event-util.h"
 #include "fd-util.h"
 #include "fs-util.h"
 #include "log.h"
 #include "parse-util.h"
 #include "path-util.h"
 #include "pidfd-util.h"
+#include "pidref.h"
 #include "process-util.h"
 #include "random-util.h"
 #include "rm-rf.h"
@@ -1167,4 +1169,31 @@ TEST(defer_fair_scheduling) {
         }
 }
 
+TEST(child_autoreap_ebusy) {
+        _cleanup_(sd_event_unrefp) sd_event *e = NULL;
+        _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL;
+        _cleanup_(pidref_done_sigkill_wait) PidRef pidref = PIDREF_NULL;
+
+        /* Test that sd_event_add_child() fails with EBUSY when kernel autoreaping is enabled
+         * by setting SIGCHLD disposition to SIG_IGN */
+
+        ASSERT_OK(sd_event_new(&e));
+
+        /* First, verify that adding a child source works with default signal disposition */
+        ASSERT_OK_POSITIVE(pidref_safe_fork("(child-autoreaping-ebusy)", FORK_DEATHSIG_SIGKILL|FORK_FREEZE, &pidref));
+
+        ASSERT_OK(event_add_child_pidref(e, &s, &pidref, WEXITED, NULL, NULL));
+        s = sd_event_source_unref(s);
+
+        /* Now set SIGCHLD to SIG_IGN to enable kernel autoreaping */
+        struct sigaction old_sa, new_sa = {};
+        new_sa.sa_handler = SIG_IGN;
+        ASSERT_OK_ERRNO(sigaction(SIGCHLD, &new_sa, &old_sa));
+
+        ASSERT_ERROR(event_add_child_pidref(e, &s, &pidref, WEXITED, NULL, NULL), EBUSY);
+
+        /* Restore original SIGCHLD disposition */
+        ASSERT_OK_ERRNO(sigaction(SIGCHLD, &old_sa, NULL));
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG);