From: Daan De Meyer Date: Thu, 18 Dec 2025 09:30:36 +0000 (+0100) Subject: sd-event: Clean up SIGCHLD conditions for sd_event_add_child() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a1e5d7f865cfd157be26b11c62efe33dcaefc219;p=thirdparty%2Fsystemd.git 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 d849c75a5fb..0d491158b84 100644 --- 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 828f0cd6f4b..a1d476243ef 100644 --- 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 28ff6245165..b0be46f03ea 100644 --- a/man/sd_event_add_child.xml +++ b/man/sd_event_add_child.xml @@ -149,11 +149,13 @@ SD_EVENT_OFF with sd_event_source_set_enabled3. - The SIGCHLD signal must be blocked in all threads before this function is - called (using sigprocmask2 or - pthread_sigmask3). + The kernel autoreaping logic must be disabled for this function to work as expected (see + signal7). + + When watching for WSTOPPED or WCONTINUED, the + SIGCHLD signal must be blocked in all threads before this function is called (using + sigprocmask2 or + pthread_sigmask3). If the second parameter of sd_event_add_child() is passed as NULL no reference to the event source object is returned. In this case, the event @@ -190,7 +192,9 @@ regardless which of sd_event_add_child() and sd_event_add_child_pidfd() 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 - SIGCHLD has to be blocked in the invoking process. + SIGCHLD has to be blocked in the invoking process when watching for + WSTOPPED or WCONTINUED and the kernel autoreaping logic has to + be disabled. sd_event_source_get_child_pid() 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 2e5f7f24494..a157462b945 100644 --- 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 78c4ae4c569..298bd409a0d 100644 --- 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 5a9c54d8e1d..8c549eb22e0 100644 --- 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 16bbd1296b1..648daf7c81d 100644 --- 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);