From 1d6dcec3864c641a5dc0a0c3ceb671cd20dcc402 Mon Sep 17 00:00:00 2001
From: Luca Boccassi <bluca@debian.org>
Date: Mon, 11 Dec 2023 01:03:39 +0000
Subject: [PATCH 0042/1160] executor: don't duplicate FD array to avoid double
 closing

Just use ExecParam directly, as these are all internal to sd-exec now
anyway. Avoids double close when execution fails after FDs are set up
for inheritance and were already re-arranged.

Fixes https://github.com/systemd/systemd/issues/30412

(cherry picked from commit 1eeaa93de36678001aeff329bc34e2b03d49f1e4)
---
 src/core/exec-invoke.c                 | 49 +++++++-------------------
 test/TEST-07-PID1/test.sh              |  2 +-
 test/units/testsuite-07.issue-30412.sh | 31 ++++++++++++++++
 3 files changed, 45 insertions(+), 37 deletions(-)
 create mode 100755 test/units/testsuite-07.issue-30412.sh

diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index cc87f213de..70d963e269 100644
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -1819,7 +1819,6 @@ static int build_environment(
                 const ExecParameters *p,
                 const CGroupContext *cgroup_context,
                 size_t n_fds,
-                char **fdnames,
                 const char *home,
                 const char *username,
                 const char *shell,
@@ -1853,7 +1852,7 @@ static int build_environment(
                         return -ENOMEM;
                 our_env[n_env++] = x;
 
-                joined = strv_join(fdnames, ":");
+                joined = strv_join(p->fd_names, ":");
                 if (!joined)
                         return -ENOMEM;
 
@@ -3718,18 +3717,11 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const
         return TAKE_FD(fd);
 }
 
-static int collect_open_file_fds(
-                const ExecContext *c,
-                const ExecParameters *p,
-                int **fds,
-                char ***fdnames,
-                size_t *n_fds) {
+static int collect_open_file_fds(const ExecContext *c, ExecParameters *p, size_t *n_fds) {
         int r;
 
         assert(c);
         assert(p);
-        assert(fds);
-        assert(fdnames);
         assert(n_fds);
 
         LIST_FOREACH(open_files, of, p->open_files) {
@@ -3745,14 +3737,14 @@ static int collect_open_file_fds(
                         return fd;
                 }
 
-                if (!GREEDY_REALLOC(*fds, *n_fds + 1))
+                if (!GREEDY_REALLOC(p->fds, *n_fds + 1))
                         return -ENOMEM;
 
-                r = strv_extend(fdnames, of->fdname);
+                r = strv_extend(&p->fd_names, of->fdname);
                 if (r < 0)
                         return r;
 
-                (*fds)[*n_fds] = TAKE_FD(fd);
+                p->fds[*n_fds] = TAKE_FD(fd);
 
                 (*n_fds)++;
         }
@@ -3959,11 +3951,9 @@ int exec_invoke(
         int secure_bits;
         _cleanup_free_ gid_t *gids_after_pam = NULL;
         int ngids_after_pam = 0;
-        _cleanup_free_ int *fds = NULL;
-        _cleanup_strv_free_ char **fdnames = NULL;
 
-        int socket_fd = -EBADF, named_iofds[3] = EBADF_TRIPLET, *params_fds = NULL;
-        size_t n_storage_fds = 0, n_socket_fds = 0;
+        int socket_fd = -EBADF, named_iofds[3] = EBADF_TRIPLET;
+        size_t n_storage_fds, n_socket_fds;
 
         assert(command);
         assert(context);
@@ -3996,8 +3986,8 @@ int exec_invoke(
                         return log_exec_error_errno(context, params, SYNTHETIC_ERRNO(EINVAL), "Got no socket.");
 
                 socket_fd = params->fds[0];
+                n_storage_fds = n_socket_fds = 0;
         } else {
-                params_fds = params->fds;
                 n_socket_fds = params->n_socket_fds;
                 n_storage_fds = params->n_storage_fds;
         }
@@ -4039,26 +4029,14 @@ int exec_invoke(
         /* In case anything used libc syslog(), close this here, too */
         closelog();
 
-        fds = newdup(int, params_fds, n_fds);
-        if (!fds) {
-                *exit_status = EXIT_MEMORY;
-                return log_oom();
-        }
-
-        fdnames = strv_copy((char**) params->fd_names);
-        if (!fdnames) {
-                *exit_status = EXIT_MEMORY;
-                return log_oom();
-        }
-
-        r = collect_open_file_fds(context, params, &fds, &fdnames, &n_fds);
+        r = collect_open_file_fds(context, params, &n_fds);
         if (r < 0) {
                 *exit_status = EXIT_FDS;
                 return log_exec_error_errno(context, params, r, "Failed to get OpenFile= file descriptors: %m");
         }
 
         int keep_fds[n_fds + 3];
-        memcpy_safe(keep_fds, fds, n_fds * sizeof(int));
+        memcpy_safe(keep_fds, params->fds, n_fds * sizeof(int));
         n_keep_fds = n_fds;
 
         r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, &params->exec_fd);
@@ -4456,7 +4434,6 @@ int exec_invoke(
                         params,
                         cgroup_context,
                         n_fds,
-                        fdnames,
                         home,
                         username,
                         shell,
@@ -4566,7 +4543,7 @@ int exec_invoke(
                  * wins here. (See above.) */
 
                 /* All fds passed in the fds array will be closed in the pam child process. */
-                r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, fds, n_fds);
+                r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, params->fds, n_fds);
                 if (r < 0) {
                         *exit_status = EXIT_PAM;
                         return log_exec_error_errno(context, params, r, "Failed to set up PAM session: %m");
@@ -4798,9 +4775,9 @@ int exec_invoke(
 
         r = close_all_fds(keep_fds, n_keep_fds);
         if (r >= 0)
-                r = shift_fds(fds, n_fds);
+                r = shift_fds(params->fds, n_fds);
         if (r >= 0)
-                r = flag_fds(fds, n_socket_fds, n_fds, context->non_blocking);
+                r = flag_fds(params->fds, n_socket_fds, n_fds, context->non_blocking);
         if (r < 0) {
                 *exit_status = EXIT_FDS;
                 return log_exec_error_errno(context, params, r, "Failed to adjust passed file descriptors: %m");
diff --git a/test/TEST-07-PID1/test.sh b/test/TEST-07-PID1/test.sh
index a5982e0183..cc8a81f77d 100755
--- a/test/TEST-07-PID1/test.sh
+++ b/test/TEST-07-PID1/test.sh
@@ -36,7 +36,7 @@ EOF
     "${SYSTEMCTL:?}" enable --root="$workspace" issue2730.mount
     ln -svrf "$workspace/etc/systemd/system/issue2730.mount" "$workspace/etc/systemd/system/issue2730-alias.mount"
 
-    image_install logger
+    image_install logger socat
 }
 
 do_test "$@"
diff --git a/test/units/testsuite-07.issue-30412.sh b/test/units/testsuite-07.issue-30412.sh
new file mode 100755
index 0000000000..333b95f9bb
--- /dev/null
+++ b/test/units/testsuite-07.issue-30412.sh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+set -eux
+set -o pipefail
+
+# Check that socket FDs are not double closed on error: https://github.com/systemd/systemd/issues/30412
+
+mkdir -p /run/systemd/system
+
+rm -f /tmp/badbin
+touch /tmp/badbin
+chmod 744 /tmp/badbin
+
+cat >/run/systemd/system/badbin_assert.service <<EOF
+[Service]
+ExecStart=/tmp/badbin
+Restart=never
+EOF
+
+cat >/run/systemd/system/badbin_assert.socket <<EOF
+[Socket]
+ListenStream=@badbin_assert.socket
+EOF
+
+systemctl daemon-reload
+systemctl start badbin_assert.socket
+
+socat - ABSTRACT-CONNECT:badbin_assert.socket
+
+timeout 10 sh -c 'while systemctl is-active badbin_assert.service; do sleep .5; done'
+[[ "$(systemctl show -P ExecMainStatus badbin_assert.service)" == 203 ]]
-- 
2.33.0