core/service: rework management of exec_fd event source

The code in service_spawn() was written as if exec_fd_event_source
was always unset. (We would either fail the assertion that is moved in the
patch, or leak the event source object if it was set.)

To make this work, let's always assert that exec_fd_event_source is unset,
and actually unset it service_sigchld_event(). I think this is the most
elegant approach. The problem is that we don't have the same information
about execution flags as in service_spawn(), so we need to conditionalize
on pid==main_pid to know if we should disable exec_fd_event_source.
I think this matches all cases where we may set exec_fd_event_source:
service_enter_start() and service_run_next_main().

service_enter_stop_post() calls service_set_state(), which will also destroy
the source. But that happens too late, because from service_enter_stop_post()
we call service_spawn() first, and then service_set_state() second.

(An alternative approach would be to deallocate the existing
exec_fd_event_source in service_spawn(). But this would mean that we would
temporarily have an event source attached to a process that we already know is
dead, which seems less than ideal.)

Original report from Dimitri John Ledkov <dimitri.ledkov@canonical.com>:
> Ubuntu private bug reference for this issue at the moment is
> https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1921145

> Michael's and Ian's team run into an issue when using systemd in the
> initrd, without dbus daemon running, and launching a unit in a
> particular way that appears to lock up systemd (pid 1) it self.

> michael vogt: "The attached script works for me to reproduce this on
> classic. I tested 20.04 (245) and 21.04 (247) in a qemu VM. Sometimes
> I need to run it multiple times but usually it crashes after at most 2
> runs. Use "journalctl | tail" to see the messages, it's the same that
> Ian reported. There is also a /var/crash/_usr_lib_systemd_systemd
> crash file created."

> I understand that the particular way to run a unit is very odd,
> however, it is currently possible to invoke, and it would be expected
> for pid1 to not lock up and crash.

> The Assertion that systemd hits is along the lines of:

> [ 10.182627] systemd[1]: Assertion 's' failed at
> src/core/service.c:3204, function service_dispatch_exec_io().
> Aborting.
> [ 10.195458] systemd[1]: Caught <ABRT>, dumped core as pid 449.
> [ 10.204446] systemd[1]: Freezing execution.

diff --git a/src/core/service.c b/src/core/service.c
index a076d5886c1f1f088debffcbeee19f0e03663fde..7114515d37a332ef80d78bd491c428c2c506641b 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1351,7 +1351,7 @@ static int service_allocate_exec_fd_event_source(
         if (r < 0)
                 return log_unit_error_errno(UNIT(s), r, "Failed to adjust priority of exec_fd event source: %m");
 
-        (void) sd_event_source_set_description(source, "service event_fd");
+        (void) sd_event_source_set_description(source, "service exec_fd");
 
         r = sd_event_source_set_io_fd_own(source, true);
         if (r < 0)
@@ -1433,6 +1433,8 @@ static int service_spawn(
         if (r < 0)
                 return r;
 
+        assert(!s->exec_fd_event_source);
+
         if (flags & EXEC_IS_CONTROL) {
                 /* If this is a control process, mask the permissions/chroot application if this is requested. */
                 if (s->permissions_start_only)
@@ -1458,8 +1460,6 @@ static int service_spawn(
         }
 
         if (!FLAGS_SET(flags, EXEC_IS_CONTROL) && s->type == SERVICE_EXEC) {
-                assert(!s->exec_fd_event_source);
-
                 r = service_allocate_exec_fd(s, &exec_fd_source, &exec_params.exec_fd);
                 if (r < 0)
                         return r;
@@ -3391,6 +3391,11 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) {
         else
                 clean_mode = EXIT_CLEAN_DAEMON;
 
+        if (s->main_pid == pid)
+                /* Clean up the exec_fd event source. The source owns its end of the pipe, so this will close
+                 * that too. */
+                s->exec_fd_event_source = sd_event_source_disable_unref(s->exec_fd_event_source);
+
         if (is_clean_exit(code, status, clean_mode, &s->success_status))
                 f = SERVICE_SUCCESS;
         else if (code == CLD_EXITED)