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.

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().

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);
 

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
+    <para>The kernel autoreaping logic must be disabled for this function to work as expected (see
-    called (using <citerefentry
+    <citerefentry project='man-pages'><refentrytitle>signal</refentrytitle><manvolnum>7</manvolnum></citerefentry>).</para>
-    project='man-pages'><refentrytitle>sigprocmask</refentrytitle><manvolnum>2</manvolnum></citerefentry> or
+
-    <citerefentry
+    <para>When watching for <constant>WSTOPPED</constant> or <constant>WCONTINUED</constant>, the
-    project='man-pages'><refentrytitle>pthread_sigmask</refentrytitle><manvolnum>3</manvolnum></citerefentry>).</para>
+    <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

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;

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)
 

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,
+                r = verify_sigchld(options);
-                 * 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);
                 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
+        /* We always take a pidfd here, even if we wait for anything else than WEXITED, so that we pin the
-         * pin the PID, and make regular waitid() handling race-free. */
+         * 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);

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);