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

diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index c73b221d743e1cc23d22970d13aba43d38836c0a..7ce26582969645ae7a3ab70d25b574b60133f124 100644 (file)
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -1807,7 +1807,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,
@@ -1841,7 +1840,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;
 
@@ -3706,18 +3705,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) {
@@ -3733,14 +3725,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)++;
         }
@@ -3947,11 +3939,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);
@@ -3984,8 +3974,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;
         }
@@ -4027,26 +4017,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);
@@ -4444,7 +4422,6 @@ int exec_invoke(
                         params,
                         cgroup_context,
                         n_fds,
-                        fdnames,
                         home,
                         username,
                         shell,
@@ -4554,7 +4531,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");
@@ -4786,9 +4763,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 a5982e0183e358b862d73f1dbf47624f1a80cd3c..cc8a81f77df567092b575c2da71f4a528248a155 100755 (executable)
--- 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 (executable)
index 0000000..333b95f
--- /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 ]]