From: Luca Boccassi <bluca@debian.org>
Date: Fri, 27 Oct 2023 15:33:49 +0000 (+0100)
Subject: exec-invoke: don't double-close FDs on error
X-Git-Tag: v255-rc1~106
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7b6d3dcdd202ab8ae0478edef6eb6fa9a5437a4b;p=thirdparty%2Fsystemd.git

exec-invoke: don't double-close FDs on error

When a late error occurs in sd-executor, the cleanup-on-close of the
context structs happen, but at that time all FDs might have already
been closed via close_all_fds(), so a double-close happens. This
can be seen when DynamicUser is enabled, with a non-existing
WorkingDirectory.

Invalidate the FDs in the context structs if close_all_fds succeeds.
---

diff --git a/src/core/dynamic-user.c b/src/core/dynamic-user.c
index 76842e5f24c..12724c682c0 100644
--- a/src/core/dynamic-user.c
+++ b/src/core/dynamic-user.c
@@ -858,3 +858,14 @@ void dynamic_creds_done(DynamicCreds *creds) {
                 dynamic_user_free(creds->group);
         creds->group = creds->user = dynamic_user_free(creds->user);
 }
+
+void dynamic_creds_close(DynamicCreds *creds) {
+        if (!creds)
+                return;
+
+        if (creds->user)
+                safe_close_pair(creds->user->storage_socket);
+
+        if (creds->group && creds->group != creds->user)
+                safe_close_pair(creds->group->storage_socket);
+}
diff --git a/src/core/dynamic-user.h b/src/core/dynamic-user.h
index e86ee02796b..303a7d081c9 100644
--- a/src/core/dynamic-user.h
+++ b/src/core/dynamic-user.h
@@ -41,6 +41,7 @@ int dynamic_creds_realize(DynamicCreds *creds, char **suggested_paths, uid_t *ui
 DynamicCreds *dynamic_creds_unref(DynamicCreds *creds);
 DynamicCreds *dynamic_creds_destroy(DynamicCreds *creds);
 void dynamic_creds_done(DynamicCreds *creds);
+void dynamic_creds_close(DynamicCreds *creds);
 
 DEFINE_TRIVIAL_CLEANUP_FUNC(DynamicCreds*, dynamic_creds_unref);
 DEFINE_TRIVIAL_CLEANUP_FUNC(DynamicCreds*, dynamic_creds_destroy);
diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index 9be39a56108..886e087bf0d 100644
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -3862,6 +3862,33 @@ static int exec_context_named_iofds(
         return targets == 0 ? 0 : -ENOENT;
 }
 
+static void exec_shared_runtime_close(ExecSharedRuntime *shared) {
+        if (!shared)
+                return;
+
+        safe_close_pair(shared->netns_storage_socket);
+        safe_close_pair(shared->ipcns_storage_socket);
+}
+
+static void exec_runtime_close(ExecRuntime *rt) {
+        if (!rt)
+                return;
+
+        safe_close_pair(rt->ephemeral_storage_socket);
+
+        exec_shared_runtime_close(rt->shared);
+        dynamic_creds_close(rt->dynamic_creds);
+}
+
+static void exec_params_close(ExecParameters *p) {
+        if (!p)
+                return;
+
+        p->stdin_fd = safe_close(p->stdin_fd);
+        p->stdout_fd = safe_close(p->stdout_fd);
+        p->stderr_fd = safe_close(p->stderr_fd);
+}
+
 int exec_invoke(
                 const ExecCommand *command,
                 const ExecContext *context,
@@ -4734,7 +4761,10 @@ int exec_invoke(
         /* We repeat the fd closing here, to make sure that nothing is leaked from the PAM modules. Note that
          * we are more aggressive this time, since we don't need socket_fd and the netns and ipcns fds any
          * more. We do keep exec_fd however, if we have it, since we need to keep it open until the final
-         * execve(). */
+         * execve(). But first, close the remaining sockets in the context objects. */
+
+        exec_runtime_close(runtime);
+        exec_params_close(params);
 
         r = close_all_fds(keep_fds, n_keep_fds);
         if (r >= 0)
diff --git a/test/units/testsuite-07.exec-context.sh b/test/units/testsuite-07.exec-context.sh
index 477b9e84dac..0943300424d 100755
--- a/test/units/testsuite-07.exec-context.sh
+++ b/test/units/testsuite-07.exec-context.sh
@@ -186,3 +186,6 @@ systemd-run --wait --pipe -p BindPaths="/etc /home:/mnt:norbind -/foo/bar/baz:/u
     bash -xec "mountpoint /etc; test -d /etc/systemd; mountpoint /mnt; ! mountpoint /usr"
 systemd-run --wait --pipe -p BindReadOnlyPaths="/etc /home:/mnt:norbind -/foo/bar/baz:/usr:rbind" \
     bash -xec "test ! -w /etc; test ! -w /mnt; ! mountpoint /usr"
+
+# Ensure that clean-up codepaths work correctly if activation ultimately fails
+(! systemd-run --wait --pipe -p DynamicUser=yes -p WorkingDirectory=/nonexistent echo hello)