From c7a444a9c1b65aa6e7e59c87fe29712f780cd86a Mon Sep 17 00:00:00 2001 From: Daniel Foster Date: Fri, 18 Jul 2025 09:59:14 +1000 Subject: [PATCH] tree-wide: extend $LISTEN_FDS protocol with $LISTEN_PIDFDID Although extremely unlikely, there is a race present in solely checking the $LISTEN_PID environment variable, due to PID recycling. Fix that by introducing $LISTEN_PIDFDID, which contains the 64-bit ID of a pidfd for the child process that is not subject to recycling. --- TODO | 4 ++-- docs/FILE_DESCRIPTOR_STORE.md | 6 +++--- man/sd_listen_fds.xml | 19 ++++++++++++------- man/systemd-socket-activate.xml | 1 + man/systemd.exec.xml | 1 + man/systemd.xml | 1 + src/core/exec-invoke.c | 10 +++++++++- src/core/manager.c | 1 + src/libsystemd/sd-daemon/sd-daemon.c | 18 ++++++++++++++++++ src/libsystemd/sd-varlink/sd-varlink.c | 13 +++++++++++++ src/mountfsd/mountfsd-manager.c | 12 ++++++++++++ src/nsresourced/nsresourced-manager.c | 12 ++++++++++++ src/socket-activate/socket-activate.c | 8 ++++++++ src/systemd/sd-daemon.h | 20 ++++++++++---------- src/userdb/userdbd-manager.c | 10 ++++++++++ src/varlinkctl/varlinkctl.c | 11 +++++++++++ 16 files changed, 124 insertions(+), 23 deletions(-) diff --git a/TODO b/TODO index 16bf6c80af4..5f973951887 100644 --- a/TODO +++ b/TODO @@ -529,8 +529,8 @@ Features: be established on top of dm-crypt or dm-verity devices, or an allowlist of file systems (which should probably include vfat, for compat with the ESP) -* $LISTEN_PID, $SYSTEMD_EXECPID env vars that the service manager sets should - be augmented with $LISTEN_PIDFDID, and $SYSTEMD_EXECPIDFD (and similar for +* $SYSTEMD_EXECPID that the service manager sets should + be augmented with $SYSTEMD_EXECPIDFD (and similar for other env vars we might send). * port copy.c over to use LabelOps for all labelling. diff --git a/docs/FILE_DESCRIPTOR_STORE.md b/docs/FILE_DESCRIPTOR_STORE.md index 757a2a27080..00b2f3f4aae 100644 --- a/docs/FILE_DESCRIPTOR_STORE.md +++ b/docs/FILE_DESCRIPTOR_STORE.md @@ -65,9 +65,9 @@ string of choice, which may be used to identify the fd later. Whenever the service is restarted the fds in its fdstore will be passed to the new instance following the same protocol as for socket activation fds. i.e. the -`$LISTEN_FDS`, `$LISTEN_PIDS`, `$LISTEN_FDNAMES` environment variables will be -set (the latter will be populated from the `FDNAME=…` field mentioned -above). See +`$LISTEN_FDS`, `$LISTEN_PID`, `$LISTEN_PIDFDID`, and `$LISTEN_FDNAMES` +environment variables will be set (the latter will be populated from the +`FDNAME=…` field mentioned above). See [`sd_listen_fds()`](https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html) for details on receiving such fds in a service. (Note that the name set in `FDNAME=…` does not need to be unique, which is useful when operating with diff --git a/man/sd_listen_fds.xml b/man/sd_listen_fds.xml index f8a6d43ffaf..19b0b0a98d3 100644 --- a/man/sd_listen_fds.xml +++ b/man/sd_listen_fds.xml @@ -93,10 +93,10 @@ If the unset_environment parameter is non-zero, sd_listen_fds() will unset the - $LISTEN_FDS, $LISTEN_PID and - $LISTEN_FDNAMES environment variables before - returning (regardless of whether the function call itself - succeeded or not). Further calls to + $LISTEN_FDS, $LISTEN_PID, + $LISTEN_PIDFDID, and $LISTEN_FDNAMES + environment variables before returning (regardless of whether the function + call itself succeeded or not). Further calls to sd_listen_fds() will then return zero, but the variables are no longer inherited by child processes. @@ -175,8 +175,9 @@ On failure, these calls returns a negative errno-style error code. If $LISTEN_FDS/$LISTEN_PID was - not set or was not correctly set for this daemon and hence no file - descriptors were received, 0 is returned. Otherwise, the number of + not set, or one of those variables or $LISTEN_PIDFDID + was not correctly set for this daemon, no file + descriptors were received and 0 is returned. Otherwise, the number of file descriptors passed is returned. The application may find them starting with file descriptor SD_LISTEN_FDS_START, i.e. file descriptor 3. @@ -190,7 +191,10 @@ Internally, sd_listen_fds() checks whether the $LISTEN_PID environment variable - equals the daemon PID. If not, it returns immediately. Otherwise, + equals the daemon PID. If not, it returns immediately. Since version 259, + if the $LISTEN_PIDFDID environment variable is set, + it also checks whether it matches the inode of this process' pidfd. + If both of these checks pass, it parses the number passed in the $LISTEN_FDS environment variable, then sets the FD_CLOEXEC flag for the parsed number of file descriptors starting from SD_LISTEN_FDS_START. @@ -209,6 +213,7 @@ $LISTEN_PID + $LISTEN_PIDFDID $LISTEN_FDS $LISTEN_FDNAMES diff --git a/man/systemd-socket-activate.xml b/man/systemd-socket-activate.xml index ac9deaf4b75..61139a02916 100644 --- a/man/systemd-socket-activate.xml +++ b/man/systemd-socket-activate.xml @@ -157,6 +157,7 @@ $LISTEN_FDS $LISTEN_PID + $LISTEN_PIDFDID $LISTEN_FDNAMES See diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index f3bb1371be0..d6ceb1490b1 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -4207,6 +4207,7 @@ StandardInputData=V2XigLJyZSBubyBzdHJhbmdlcnMgdG8gbG92ZQpZb3Uga25vdyB0aGUgcnVsZX $LISTEN_FDS $LISTEN_PID + $LISTEN_PIDFDID $LISTEN_FDNAMES Information about file descriptors passed to a diff --git a/man/systemd.xml b/man/systemd.xml index 857c942eed1..06d9102e475 100644 --- a/man/systemd.xml +++ b/man/systemd.xml @@ -720,6 +720,7 @@ $LISTEN_PID + $LISTEN_PIDFDID $LISTEN_FDS $LISTEN_FDNAMES diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 8042dbcd77e..de59e1148ec 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -59,6 +59,7 @@ #include "osc-context.h" #include "pam-util.h" #include "path-util.h" +#include "pidfd-util.h" #include "pidref.h" #include "proc-cmdline.h" #include "process-util.h" @@ -2014,7 +2015,7 @@ static int build_environment( assert(cgroup_context); assert(ret); -#define N_ENV_VARS 19 +#define N_ENV_VARS 20 our_env = new0(char*, N_ENV_VARS + _EXEC_DIRECTORY_TYPE_MAX + 1); if (!our_env) return -ENOMEM; @@ -2026,6 +2027,13 @@ static int build_environment( return -ENOMEM; our_env[n_env++] = x; + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + if (asprintf(&x, "LISTEN_PIDFDID=%"PRIu64, pidfdid) < 0) + return -ENOMEM; + our_env[n_env++] = x; + } + if (asprintf(&x, "LISTEN_FDS=%zu", n_fds) < 0) return -ENOMEM; our_env[n_env++] = x; diff --git a/src/core/manager.c b/src/core/manager.c index 520434993f8..90a3f25aebd 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -623,6 +623,7 @@ static char** sanitize_environment(char **l) { "LISTEN_FDNAMES", "LISTEN_FDS", "LISTEN_PID", + "LISTEN_PIDFDID", "LOGS_DIRECTORY", "LOG_NAMESPACE", "MAINPID", diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 0cf3cbdd4cd..b5fe6dfc120 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -35,6 +35,7 @@ static void unsetenv_listen(bool unset_environment) { return; assert_se(unsetenv("LISTEN_PID") == 0); + assert_se(unsetenv("LISTEN_PIDFDID") == 0); assert_se(unsetenv("LISTEN_FDS") == 0); assert_se(unsetenv("LISTEN_FDNAMES") == 0); } @@ -60,6 +61,23 @@ _public_ int sd_listen_fds(int unset_environment) { goto finish; } + e = getenv("LISTEN_PIDFDID"); + if (e) { + uint64_t own_pidfdid, pidfdid; + + r = safe_atou64(e, &pidfdid); + if (r < 0) + goto finish; + + if (pidfd_get_inode_id_self_cached(&own_pidfdid) >= 0) { + /* Is this *really* for us? */ + if (pidfdid != own_pidfdid) { + r = 0; + goto finish; + } + } + } + e = getenv("LISTEN_FDS"); if (!e) { r = 0; diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 47d110ffdfd..7a5d2008c97 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -24,6 +24,7 @@ #include "log.h" #include "mkdir.h" #include "path-util.h" +#include "pidfd-util.h" #include "process-util.h" #include "socket-util.h" #include "string-table.h" @@ -266,6 +267,18 @@ _public_ int sd_varlink_connect_exec(sd_varlink **ret, const char *_command, cha xsprintf(spid, PID_FMT, pid); + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + char spidfdid[DECIMAL_STR_MAX(uint64_t)+1]; + xsprintf(spidfdid, "%" PRIu64, pidfdid); + + if (setenv("LISTEN_PIDFDID", spidfdid, /* override= */ true) < 0) { + log_debug_errno(errno, + "Failed to set environment variable 'LISTEN_PIDFDID': %m"); + _exit(EXIT_FAILURE); + } + } + STRV_FOREACH_PAIR(a, b, setenv_list) { if (setenv(*a, *b, /* override= */ true) < 0) { log_debug_errno(errno, "Failed to set environment variable '%s': %m", *a); diff --git a/src/mountfsd/mountfsd-manager.c b/src/mountfsd/mountfsd-manager.c index c81671da32d..89e5a5aca2c 100644 --- a/src/mountfsd/mountfsd-manager.c +++ b/src/mountfsd/mountfsd-manager.c @@ -14,6 +14,7 @@ #include "format-util.h" #include "log.h" #include "mountfsd-manager.h" +#include "pidfd-util.h" #include "process-util.h" #include "set.h" #include "signal-util.h" @@ -165,6 +166,17 @@ static int start_one_worker(Manager *m) { _exit(EXIT_FAILURE); } + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + char pidfdids[DECIMAL_STR_MAX(uint64_t)]; + + xsprintf(pidfdids, "%" PRIu64, pidfdid); + if (setenv("LISTEN_PIDFDID", pidfdids, 1) < 0) { + log_error_errno(errno, "Failed to set $LISTEN_PIDFDID: %m"); + _exit(EXIT_FAILURE); + } + } + if (setenv("LISTEN_FDS", "1", 1) < 0) { log_error_errno(errno, "Failed to set $LISTEN_FDS: %m"); _exit(EXIT_FAILURE); diff --git a/src/nsresourced/nsresourced-manager.c b/src/nsresourced/nsresourced-manager.c index d58d88c7afd..e57797503b2 100644 --- a/src/nsresourced/nsresourced-manager.c +++ b/src/nsresourced/nsresourced-manager.c @@ -20,6 +20,7 @@ #include "mkdir.h" #include "nsresourced-manager.h" #include "parse-util.h" +#include "pidfd-util.h" #include "process-util.h" #include "recurse-dir.h" #include "set.h" @@ -201,6 +202,17 @@ static int start_one_worker(Manager *m) { _exit(EXIT_FAILURE); } + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + char pidfdids[DECIMAL_STR_MAX(uint64_t)]; + + xsprintf(pidfdids, "%" PRIu64, pidfdid); + if (setenv("LISTEN_PIDFDID", pidfdids, 1) < 0) { + log_error_errno(errno, "Failed to set $LISTEN_PIDFDID: %m"); + _exit(EXIT_FAILURE); + } + } + if (setenv("LISTEN_FDS", "1", 1) < 0) { log_error_errno(errno, "Failed to set $LISTEN_FDS: %m"); _exit(EXIT_FAILURE); diff --git a/src/socket-activate/socket-activate.c b/src/socket-activate/socket-activate.c index 8e5dfb41d4b..29940ac354c 100644 --- a/src/socket-activate/socket-activate.c +++ b/src/socket-activate/socket-activate.c @@ -17,6 +17,7 @@ #include "format-util.h" #include "log.h" #include "main-func.h" +#include "pidfd-util.h" #include "pretty-print.h" #include "process-util.h" #include "socket-netlink.h" @@ -186,6 +187,13 @@ static int exec_process(char * const *argv, int start_fd, size_t n_fds) { if (r < 0) return r; + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + r = strv_extendf(&envp, "LISTEN_PIDFDID=%" PRIu64, pidfdid); + if (r < 0) + return r; + } + if (arg_fdnames) { _cleanup_free_ char *names = NULL; size_t len; diff --git a/src/systemd/sd-daemon.h b/src/systemd/sd-daemon.h index b6ec7346564..de5fad7b80b 100644 --- a/src/systemd/sd-daemon.h +++ b/src/systemd/sd-daemon.h @@ -55,16 +55,16 @@ _SD_BEGIN_DECLARATIONS; /* Returns how many file descriptors have been passed, or a negative - errno code on failure. Optionally, removes the $LISTEN_FDS and - $LISTEN_PID file descriptors from the environment (recommended, but - problematic in threaded environments). If r is the return value of - this function you'll find the file descriptors passed as fds - SD_LISTEN_FDS_START to SD_LISTEN_FDS_START+r-1. Returns a negative - errno style error code on failure. This function call ensures that - the FD_CLOEXEC flag is set for the passed file descriptors, to make - sure they are not passed on to child processes. If FD_CLOEXEC shall - not be set, the caller needs to unset it after this call for all file - descriptors that are used. + errno code on failure. Optionally, removes the $LISTEN_FDS, + $LISTEN_PID, $LISTEN_PIDFDID, and $LISTEN_FDNAMES variables from the + environment (recommended, but problematic in threaded environments). + If r is the return value of this function you'll find the file + descriptors passed as fds SD_LISTEN_FDS_START to + SD_LISTEN_FDS_START+r-1. Returns a negative errno style error code on + failure. This function call ensures that the FD_CLOEXEC flag is set + for the passed file descriptors, to make sure they are not passed on + to child processes. If FD_CLOEXEC shall not be set, the caller needs + to unset it after this call for all file descriptors that are used. See sd_listen_fds(3) for more information. */ diff --git a/src/userdb/userdbd-manager.c b/src/userdb/userdbd-manager.c index 1c62bb1697b..91fa09d3166 100644 --- a/src/userdb/userdbd-manager.c +++ b/src/userdb/userdbd-manager.c @@ -15,6 +15,7 @@ #include "fs-util.h" #include "log.h" #include "mkdir.h" +#include "pidfd-util.h" #include "process-util.h" #include "set.h" #include "signal-util.h" @@ -181,6 +182,15 @@ static int start_one_worker(Manager *m) { _exit(EXIT_FAILURE); } + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + r = setenvf("LISTEN_PIDFDID", true, "%" PRIu64, pidfdid); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_PIDFDID: %m"); + _exit(EXIT_FAILURE); + } + } + if (setenv("LISTEN_FDS", "1", 1) < 0) { log_error_errno(errno, "Failed to set $LISTEN_FDS: %m"); _exit(EXIT_FAILURE); diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index 7768ca52058..865235bb30c 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -20,6 +20,7 @@ #include "pager.h" #include "parse-argument.h" #include "parse-util.h" +#include "pidfd-util.h" #include "pretty-print.h" #include "process-util.h" #include "string-util.h" @@ -876,9 +877,19 @@ static int verb_call(int argc, char *argv[], void *userdata) { log_error_errno(r, "Failed to set $LISTEN_PID environment variable: %m"); _exit(EXIT_FAILURE); } + + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + r = setenvf("LISTEN_PIDFDID", /* overwrite= */ true, "%" PRIu64, pidfdid); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_PIDFDID environment variable: %m"); + _exit(EXIT_FAILURE); + } + } } else { (void) unsetenv("LISTEN_FDS"); (void) unsetenv("LISTEN_PID"); + (void) unsetenv("LISTEN_PIDFDID"); } (void) unsetenv("LISTEN_FDNAMES"); -- 2.47.3