From 66063489818de412d558292b38c4885ffee57a1f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Nov 2024 10:46:37 +0100 Subject: [PATCH 1/4] sd-daemon: clean up env var unsetting This cleans up the handling of the "unset_environment" parameter to sd_listen() and related calls: the man pages claim we operate on it on error too. Hence, actually do so in strictly all error paths. Previously we'd miss out on some, because wrapper functions mishandled them. This was addressed before in 362dcfc5db0271cd6b3a564c528cabf0ac0e7993 but some codepaths were missed. Complete the work now. This establishes a common pattern: a function to unset the relevant env vars, that is called from a goto section at the botom on both success and failure. --- src/libsystemd/sd-daemon/sd-daemon.c | 113 ++++++++++++++++----------- 1 file changed, 68 insertions(+), 45 deletions(-) diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 1e28a45fde..ae890b6d20 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -30,7 +30,7 @@ #define SNDBUF_SIZE (8*1024*1024) -static void unsetenv_all(bool unset_environment) { +static void unsetenv_listen(bool unset_environment) { if (!unset_environment) return; @@ -85,25 +85,25 @@ _public_ int sd_listen_fds(int unset_environment) { r = n; finish: - unsetenv_all(unset_environment); + unsetenv_listen(unset_environment); return r; } -_public_ int sd_listen_fds_with_names(int unset_environment, char ***names) { +_public_ int sd_listen_fds_with_names(int unset_environment, char ***ret_names) { _cleanup_strv_free_ char **l = NULL; bool have_names; int n_names = 0, n_fds; const char *e; int r; - if (!names) + if (!ret_names) return sd_listen_fds(unset_environment); e = getenv("LISTEN_FDNAMES"); if (e) { n_names = strv_split_full(&l, e, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (n_names < 0) { - unsetenv_all(unset_environment); + unsetenv_listen(unset_environment); return n_names; } @@ -124,8 +124,7 @@ _public_ int sd_listen_fds_with_names(int unset_environment, char ***names) { return r; } - *names = TAKE_PTR(l); - + *ret_names = TAKE_PTR(l); return n_fds; } @@ -611,6 +610,13 @@ static int pid_notify_with_fds_internal( return 1; } +static void unsetenv_notify(bool unset_environment) { + if (!unset_environment) + return; + + assert_se(unsetenv("NOTIFY_SOCKET") == 0); +} + _public_ int sd_pid_notify_with_fds( pid_t pid, int unset_environment, @@ -621,10 +627,7 @@ _public_ int sd_pid_notify_with_fds( int r; r = pid_notify_with_fds_internal(pid, state, fds, n_fds); - - if (unset_environment) - assert_se(unsetenv("NOTIFY_SOCKET") == 0); - + unsetenv_notify(unset_environment); return r; } @@ -632,22 +635,28 @@ _public_ int sd_pid_notify_barrier(pid_t pid, int unset_environment, uint64_t ti _cleanup_close_pair_ int pipe_fd[2] = EBADF_PAIR; int r; - if (pipe2(pipe_fd, O_CLOEXEC) < 0) - return -errno; + r = RET_NERRNO(pipe2(pipe_fd, O_CLOEXEC)); + if (r < 0) + goto finish; - r = sd_pid_notify_with_fds(pid, unset_environment, "BARRIER=1", &pipe_fd[1], 1); + r = pid_notify_with_fds_internal(pid, "BARRIER=1", &pipe_fd[1], 1); if (r <= 0) - return r; + goto finish; pipe_fd[1] = safe_close(pipe_fd[1]); r = fd_wait_for_event(pipe_fd[0], 0 /* POLLHUP is implicit */, timeout); if (r < 0) - return r; - if (r == 0) - return -ETIMEDOUT; + goto finish; + if (r == 0) { + r = -ETIMEDOUT; + goto finish; + } - return 1; + r = 1; +finish: + unsetenv_notify(unset_environment); + return r; } _public_ int sd_notify_barrier(int unset_environment, uint64_t timeout) { @@ -664,7 +673,7 @@ _public_ int sd_notify(int unset_environment, const char *state) { _public_ int sd_pid_notifyf(pid_t pid, int unset_environment, const char *format, ...) { _cleanup_free_ char *p = NULL; - int r = 0, k; + int r; if (format) { va_list ap; @@ -673,20 +682,22 @@ _public_ int sd_pid_notifyf(pid_t pid, int unset_environment, const char *format r = vasprintf(&p, format, ap); va_end(ap); - if (r < 0 || !p) { + if (r < 0) { r = -ENOMEM; - p = mfree(p); /* If vasprintf failed, do not use the string, - * even if something was returned. */ + goto finish; } } - k = sd_pid_notify(pid, unset_environment, p); - return r < 0 ? r : k; + r = pid_notify_with_fds_internal(pid, p, /* fds= */ NULL, /* n_fds= */ 0); + +finish: + unsetenv_notify(unset_environment); + return r; } _public_ int sd_notifyf(int unset_environment, const char *format, ...) { _cleanup_free_ char *p = NULL; - int r = 0, k; + int r; if (format) { va_list ap; @@ -695,15 +706,17 @@ _public_ int sd_notifyf(int unset_environment, const char *format, ...) { r = vasprintf(&p, format, ap); va_end(ap); - if (r < 0 || !p) { + if (r < 0) { r = -ENOMEM; - p = mfree(p); /* If vasprintf failed, do not use the string, - * even if something was returned. */ + goto finish; } } - k = sd_pid_notify(0, unset_environment, p); - return r < 0 ? r : k; + r = pid_notify_with_fds_internal(/* pid= */ 0, p, /* fds= */ NULL, /* n_fds= */ 0); + +finish: + unsetenv_notify(unset_environment); + return r; } _public_ int sd_pid_notifyf_with_fds( @@ -713,14 +726,16 @@ _public_ int sd_pid_notifyf_with_fds( const char *format, ...) { _cleanup_free_ char *p = NULL; - int r = 0, k; + int r; /* Paranoia check: we traditionally used 'unsigned' as array size, but we nowadays more correctly use * 'size_t'. sd_pid_notifyf_with_fds() and sd_pid_notify_with_fds() are from different eras, hence * differ in this. Let's catch resulting incompatibilites early, even though they are pretty much * theoretic only */ - if (n_fds > UINT_MAX) + if (n_fds > UINT_MAX) { r = -E2BIG; + goto finish; + } else if (format) { va_list ap; @@ -729,15 +744,17 @@ _public_ int sd_pid_notifyf_with_fds( r = vasprintf(&p, format, ap); va_end(ap); - if (r < 0 || !p) { + if (r < 0) { r = -ENOMEM; - p = mfree(p); /* If vasprintf failed, do not use the string, - * even if something was returned. */ + goto finish; } } - k = sd_pid_notify_with_fds(pid, unset_environment, p, fds, n_fds); - return r < 0 ? r : k; + r = pid_notify_with_fds_internal(pid, p, fds, n_fds); + +finish: + unsetenv_notify(unset_environment); + return r; } _public_ int sd_booted(void) { @@ -755,14 +772,24 @@ _public_ int sd_booted(void) { return r; } +static void unsetenv_watchdog(bool unset_environment) { + if (!unset_environment) + return; + + assert_se(unsetenv("WATCHDOG_USEC") == 0); + assert_se(unsetenv("WATCHDOG_PID") == 0); +} + _public_ int sd_watchdog_enabled(int unset_environment, uint64_t *usec) { const char *s, *p = ""; /* p is set to dummy value to do unsetting */ uint64_t u; - int r = 0; + int r; s = getenv("WATCHDOG_USEC"); - if (!s) + if (!s) { + r = 0; goto finish; + } r = safe_atou64(s, &u); if (r < 0) @@ -793,10 +820,6 @@ _public_ int sd_watchdog_enabled(int unset_environment, uint64_t *usec) { r = 1; finish: - if (unset_environment && s) - assert_se(unsetenv("WATCHDOG_USEC") == 0); - if (unset_environment && p) - assert_se(unsetenv("WATCHDOG_PID") == 0); - + unsetenv_watchdog(unset_environment); return r; } From bea2237f6709aa8c432356c61440873c879483e8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Nov 2024 11:01:07 +0100 Subject: [PATCH 2/4] sd-daemon: drop some redundant 'else' --- src/libsystemd/sd-daemon/sd-daemon.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index ae890b6d20..26e4257a1f 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -180,10 +180,10 @@ _public_ int sd_is_special(int fd, const char *path) { if (S_ISREG(st_fd.st_mode) && S_ISREG(st_path.st_mode)) return stat_inode_same(&st_path, &st_fd); - else if (S_ISCHR(st_fd.st_mode) && S_ISCHR(st_path.st_mode)) + if (S_ISCHR(st_fd.st_mode) && S_ISCHR(st_path.st_mode)) return st_path.st_rdev == st_fd.st_rdev; - else - return 0; + + return 0; } return 1; From 74806f71168baca9233bf70446a704644ad8febc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Nov 2024 11:01:23 +0100 Subject: [PATCH 3/4] sd-daemon: count array elements in size_t We fucked that up in the original sd_listen() calls, and then we fixed that on the newer flavours. But pour internal common implementation should of course use the full range size_t, as it should be. This then allows us to drop a redundant range check. --- src/libsystemd/sd-daemon/sd-daemon.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 26e4257a1f..faf45c2750 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -457,7 +457,7 @@ static int pid_notify_with_fds_internal( pid_t pid, const char *state, const int *fds, - unsigned n_fds) { + size_t n_fds) { SocketAddress address; struct iovec iovec; @@ -728,16 +728,7 @@ _public_ int sd_pid_notifyf_with_fds( _cleanup_free_ char *p = NULL; int r; - /* Paranoia check: we traditionally used 'unsigned' as array size, but we nowadays more correctly use - * 'size_t'. sd_pid_notifyf_with_fds() and sd_pid_notify_with_fds() are from different eras, hence - * differ in this. Let's catch resulting incompatibilites early, even though they are pretty much - * theoretic only */ - if (n_fds > UINT_MAX) { - r = -E2BIG; - goto finish; - } - - else if (format) { + if (format) { va_list ap; va_start(ap, format); From cb42df5310e701b751331ae62432e3fb0df2f660 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Nov 2024 11:18:29 +0100 Subject: [PATCH 4/4] sd-daemon: add fd array size safety check to sd_notify_with_fds() The previous commit removed the UINT_MAX check for the fd array. Let's now re-add one, but at a better place, and with a more useful limit. As it turns out the kernel does not allow passing more than 253 fds at the same time, hence use that as limit. And do so immediately before calculating the control buffer size, so that we catch multiplication overflows. --- man/sd_notify.xml | 20 ++++++++++++++++++++ src/basic/missing_socket.h | 7 +++++++ src/libsystemd/sd-daemon/sd-daemon.c | 6 ++++++ src/notify/notify.c | 2 ++ 4 files changed, 35 insertions(+) diff --git a/man/sd_notify.xml b/man/sd_notify.xml index 5e6c9d6dbc..0c993e31f7 100644 --- a/man/sd_notify.xml +++ b/man/sd_notify.xml @@ -499,6 +499,26 @@ successfully. Specifically, no error is returned when a file descriptor is attempted to be stored using FDSTORE=1 but the service is not actually configured to permit storing of file descriptors (see above). + + + Errors + + Returned errors may indicate the following problems: + + + + -E2BIG + + More file descriptors passed at once than the system allows. On Linux the number of + file descriptors that may be passed across AF_UNIX sockets at once is 253, see + unix7 for + details. + + + + + diff --git a/src/basic/missing_socket.h b/src/basic/missing_socket.h index 55de40a321..8460ce13bf 100644 --- a/src/basic/missing_socket.h +++ b/src/basic/missing_socket.h @@ -68,3 +68,10 @@ #ifndef IPV6_RECVFRAGSIZE #define IPV6_RECVFRAGSIZE 77 #endif + +/* The maximum number of fds that SCM_RIGHTS accepts. This is an internal kernel constant, but very much + * useful for userspace too. It's documented in unix(7) these days, hence should be fairly reliable to define + * here. */ +#ifndef SCM_MAX_FD +#define SCM_MAX_FD 253U +#endif diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index faf45c2750..4d3d678cb4 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -472,6 +472,12 @@ static int pid_notify_with_fds_internal( assert_return(state, -EINVAL); assert_return(fds || n_fds == 0, -EINVAL); + /* Let's make sure the multiplications below don't overflow, and also return a recognizable error in + * case the caller tries to send more fds than the kernel limit. The kernel would return EINVAL which + * is not too useful I'd say. */ + if (n_fds > SCM_MAX_FD) + return -E2BIG; + const char *e = getenv("NOTIFY_SOCKET"); if (!e) return 0; diff --git a/src/notify/notify.c b/src/notify/notify.c index 8937457ec9..6afb9560c6 100644 --- a/src/notify/notify.c +++ b/src/notify/notify.c @@ -453,6 +453,8 @@ static int run(int argc, char* argv[]) { r = sd_pid_notify_with_fds(arg_pid.pid, /* unset_environment= */ false, msg, a, k); } + if (r == -E2BIG) + return log_error_errno(r, "Too many file descriptors passed."); if (r < 0) return log_error_errno(r, "Failed to notify init system: %m"); if (r == 0)