fork-notify: Use callback instead of argv NULL code path with return

In 012d87c1fc/cc8f398202 it was made possible for fork_notify() to
return in the child but at that point all FDs were closed and the
_cleanup path from the return causes assertion failures due to invalid
FDs in notify_event_source/event, leading to a vmspawn failing to start
with a SIGABRT logged in coredump.
Instead of TAKE_PTR on a bunch of things which is fragile, rather avoid
the return and instead add an explicit callback handler and guarantee
to exit directly after it. A userdata argument is also added but not
used yet I think it's quite normal to have for a callback.

diff --git a/src/shared/fork-notify.c b/src/shared/fork-notify.c
index 066ee29115faa81b56234ca7074dd83974e49691..0a34c491b7aa51bf92719f48372cb31e79bf217e 100644 (file)
--- a/src/shared/fork-notify.c
+++ b/src/shared/fork-notify.c
@@ -92,9 +92,10 @@ static int on_child_notify(sd_event_source *s, int fd, uint32_t revents, void *u
         return 0;
 }
 
-int fork_notify(char * const *argv, PidRef *ret_pidref) {
+int fork_notify(char * const *argv, fork_notify_handler_t child_handler, void *child_userdata, PidRef *ret_pidref) {
         int r;
 
+        assert(argv);
         assert(ret_pidref);
 
         if (!is_main_thread())
@@ -123,7 +124,7 @@ int fork_notify(char * const *argv, PidRef *ret_pidref) {
         if (r < 0)
                 return r;
 
-        if (DEBUG_LOGGING && argv) {
+        if (DEBUG_LOGGING) {
                 _cleanup_free_ char *l = quote_command_line(argv, SHELL_ESCAPE_EMPTY);
                 log_debug("Invoking '%s' as child.", strnull(l));
         }
@@ -145,10 +146,11 @@ int fork_notify(char * const *argv, PidRef *ret_pidref) {
                         _exit(EXIT_MEMORY);
                 }
 
-                if (!argv) {
-                        *ret_pidref = TAKE_PIDREF(child);
-                        return 0; /* Let the caller run custom code in the child */
-                }
+                /* After fork and before exec one can execute custom code in this function
+                 * but since all open FDs were closed only limited actions are safe (e.g., setenv),
+                 * akin to how in a signal handler only certain things are safe. */
+                if (child_handler)
+                        child_handler(child_userdata);
 
                 r = invoke_callout_binary(argv[0], argv);
                 log_debug_errno(r, "Failed to invoke %s: %m", argv[0]);
@@ -173,7 +175,7 @@ int fork_notify(char * const *argv, PidRef *ret_pidref) {
 
         *ret_pidref = TAKE_PIDREF(child);
 
-        return 1; /* In the parent */
+        return 0;
 }
 
 static void fork_notify_terminate_internal(PidRef *pidref) {
@@ -241,7 +243,14 @@ int journal_fork(RuntimeScope scope, char * const* units, OutputMode output, Pid
                 if (strv_extendf(&argv, "--output=%s", output_mode_to_string(output)) < 0)
                         return log_oom_debug();
 
-        return fork_notify(argv, ret_pidref);
+        return fork_notify(argv, /* child_handler= */ NULL, /* child_userdata= */ NULL, ret_pidref);
+}
+
+static void set_journal_remote_config(void *userdata) {
+        if (setenv("SYSTEMD_JOURNAL_REMOTE_CONFIG_FILE", "/dev/null", /* overwrite= */ true) < 0) {
+                log_debug_errno(errno, "Failed to set $SYSTEMD_JOURNAL_REMOTE_CONFIG_FILE: %m");
+                _exit(EXIT_MEMORY);
+        }
 }
 
 int fork_journal_remote(
@@ -313,22 +322,5 @@ int fork_journal_remote(
             strv_extendf(&argv, "--max-files=%" PRIu64, max_files) < 0)
                 return log_oom();
 
-        r = fork_notify(/* argv= */ NULL, ret_pidref);
-        if (r < 0)
-                return r;
-        if (r == 0) {
-                /* In the child */
-                if (setenv("SYSTEMD_JOURNAL_REMOTE_CONFIG_FILE",
-                            "/dev/null",
-                            /* overwrite= */ true) < 0) {
-                        log_debug_errno(errno, "Failed to set $SYSTEMD_JOURNAL_REMOTE_CONFIG_FILE: %m");
-                        _exit(EXIT_MEMORY);
-                }
-
-                r = invoke_callout_binary(argv[0], argv);
-                log_debug_errno(r, "Failed to invoke %s: %m", argv[0]);
-                _exit(EXIT_EXEC);
-        }
-
-        return 0;
+        return fork_notify(argv, set_journal_remote_config, /* child_userdata= */ NULL, ret_pidref);
 }

diff --git a/src/shared/fork-notify.h b/src/shared/fork-notify.h
index cc241beff9335b9981b5ceb42dcab5c1b48b9268..cf22f48ba2ace0d47f99e4756cc393a517899f76 100644 (file)
--- a/src/shared/fork-notify.h
+++ b/src/shared/fork-notify.h
@@ -4,7 +4,9 @@
 #include "output-mode.h"
 #include "shared-forward.h"
 
-int fork_notify(char * const *argv, PidRef *ret_pidref);
+typedef void (*fork_notify_handler_t)(void *userdata);
+
+int fork_notify(char * const *argv, fork_notify_handler_t child_handler, void *child_userdata, PidRef *ret_pidref);
 
 void fork_notify_terminate(PidRef *pidref);
 

diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c
index 1a349a1d634f34f39f3b45c1b5824ca62e905fc4..d7784e1e9a9d94dfec4af40017050e24cf3de33b 100644 (file)
--- a/src/vmspawn/vmspawn.c
+++ b/src/vmspawn/vmspawn.c
@@ -1565,7 +1565,7 @@ static int start_tpm(
         if (r < 0)
                 return log_oom();
 
-        r = fork_notify(argv, ret_pidref);
+        r = fork_notify(argv, /* child_handler= */ NULL, /* child_userdata= */ NULL, ret_pidref);
         if (r < 0)
                 return r;