From 74cd56d31b9673d2a3248e6ffbea2294a0fe2790 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 21 Feb 2025 15:16:46 +0100 Subject: [PATCH 1/3] notify-recv: several followups Follow-up for 7f6af95dab037e7d15591a924dbf256460bbf069 - Allocate internal buf on the stack, memdup() only at the end. This ensures we're able to handle OOM gracefully, i.e. return -EAGAIN on OOM while still emptying socket buffer. - Do not treat empty notify message as error. - Raise log level since all callers log loudly anyway. --- src/import/importd.c | 2 +- src/notify/notify.c | 2 +- src/nspawn/nspawn.c | 2 +- src/shared/notify-recv.c | 78 +++++++++++++++++------------- src/sysupdate/sysupdate-transfer.c | 2 +- src/sysupdate/sysupdated.c | 2 +- 6 files changed, 49 insertions(+), 39 deletions(-) diff --git a/src/import/importd.c b/src/import/importd.c index 5a1dcb92a5..21c6d969bc 100644 --- a/src/import/importd.c +++ b/src/import/importd.c @@ -655,7 +655,7 @@ static int manager_on_notify(sd_event_source *s, int fd, uint32_t revents, void if (r == -EAGAIN) return 0; if (r < 0) - return log_warning_errno(r, "Failed to receive notification message: %m"); + return r; Transfer *t; HASHMAP_FOREACH(t, m->transfers) diff --git a/src/notify/notify.c b/src/notify/notify.c index a2c53005aa..b736bf704f 100644 --- a/src/notify/notify.c +++ b/src/notify/notify.c @@ -423,7 +423,7 @@ static int on_notify_socket(sd_event_source *s, int fd, unsigned event, void *us if (r == -EAGAIN) return 0; if (r < 0) - return log_error_errno(r, "Failed to receive notification message: %m"); + return r; if (!pidref_equal(child, &pidref)) { log_warning("Received notification message from unexpected process " PID_FMT " (expected " PID_FMT "), ignoring.", diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 4ba3694280..066156a5ca 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -4626,7 +4626,7 @@ static int nspawn_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t r if (r == -EAGAIN) return 0; if (r < 0) - return log_error_errno(r, "Failed to receive notification message: %m"); + return r; if (sender_pid.pid != inner_child_pid) { log_debug("Received notify message from process that is not the payload's PID 1. Ignoring."); diff --git a/src/shared/notify-recv.c b/src/shared/notify-recv.c index c3596d0258..8730e88080 100644 --- a/src/shared/notify-recv.c +++ b/src/shared/notify-recv.c @@ -4,11 +4,20 @@ #include "notify-recv.h" #include "socket-util.h" -int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pidref) { +int notify_recv( + int fd, + char **ret_text, + struct ucred *ret_ucred, + PidRef *ret_pidref) { + + char buf[NOTIFY_BUFFER_MAX]; + struct iovec iovec = { + .iov_base = buf, + .iov_len = sizeof(buf), + }; CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred)) + CMSG_SPACE(sizeof(int)) + /* SCM_PIDFD */ CMSG_SPACE(sizeof(int) * NOTIFY_FD_MAX)) control; - struct iovec iovec; struct msghdr msghdr = { .msg_iov = &iovec, .msg_iovlen = 1, @@ -16,38 +25,32 @@ int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pi .msg_controllen = sizeof(control), }; ssize_t n; + int r; assert(fd >= 0); - /* Receives a $NOTIFY_SOCKET message (aka sd_notify()). Does various validations. Returns -EAGAIN in - * case an invalid message is received (following the logic that an invalid message shall be ignored, - * and treated like no message at all). */ - - _cleanup_free_ char *buf = new(char, NOTIFY_BUFFER_MAX+1); - if (!buf) - return log_oom_debug(); - - iovec = (struct iovec) { - .iov_base = buf, - .iov_len = NOTIFY_BUFFER_MAX, - }; + /* Receives a $NOTIFY_SOCKET message (aka sd_notify()). Does various validations. + * + * Returns -EAGAIN on recoverable errors (e.g. in case an invalid message is received, following + * the logic that an invalid message shall be ignored, and treated like no message at all). */ n = recvmsg_safe(fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC); if (ERRNO_IS_NEG_TRANSIENT(n)) return -EAGAIN; if (n == -ECHRNG) { - log_debug_errno(n, "Got message with truncated control data (unexpected fds sent?), ignoring."); + log_warning_errno(n, "Got message with truncated control data (unexpected fds sent?), ignoring."); return -EAGAIN; } if (n == -EXFULL) { - log_debug_errno(n, "Got message with truncated payload data, ignoring."); + log_warning_errno(n, "Got message with truncated payload data, ignoring."); return -EAGAIN; } if (n < 0) - return (int) n; + return log_error_errno(n, "Failed to receive notification message: %m"); const struct ucred *ucred = NULL; _cleanup_close_ int pidfd = -EBADF; + struct cmsghdr *cmsg; CMSG_FOREACH(cmsg, &msghdr) { if (cmsg->cmsg_level != SOL_SOCKET) @@ -63,28 +66,43 @@ int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pi case SCM_PIDFD: assert(cmsg->cmsg_len == CMSG_LEN(sizeof(int))); + assert(pidfd < 0); + pidfd = *CMSG_TYPED_DATA(cmsg, int); break; case SCM_CREDENTIALS: assert(cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))); + assert(!ucred); + ucred = CMSG_TYPED_DATA(cmsg, struct ucred); break; } } - if ((ret_ucred || ret_pidref) && (!ucred || ucred->pid <= 0)) - return log_debug_errno(SYNTHETIC_ERRNO(EAGAIN), "Got notification datagram lacking valid credential information, ignoring."); + if ((ret_ucred || ret_pidref) && (!ucred || !pid_is_valid(ucred->pid))) + return log_warning_errno(SYNTHETIC_ERRNO(EAGAIN), + "Got notification datagram lacking valid credential information, ignoring."); - if (n == 0) - return log_debug_errno(SYNTHETIC_ERRNO(EAGAIN), "Got empty notification message, ignoring."); - if (memchr(buf, 0, n - 1)) - return log_debug_errno(SYNTHETIC_ERRNO(EAGAIN), "Got notification message with embedded NUL, ignoring."); + /* As extra safety check, let's make sure the string we get doesn't contain embedded NUL bytes. + * We permit one trailing NUL byte in the message, but don't expect it. */ + if (n > 1 && memchr(buf, 0, n - 1)) + return log_warning_errno(SYNTHETIC_ERRNO(EAGAIN), "Got notification message with embedded NUL, ignoring."); + + if (ret_text) { + char *s = memdup_suffix0(buf, n); + if (!s) { + log_oom_warning(); + return -EAGAIN; + } + + *ret_text = s; + } + + if (ret_ucred) + *ret_ucred = *ucred; if (ret_pidref) { - assert(ucred); - assert(ucred->pid > 0); - if (pidfd >= 0) *ret_pidref = (PidRef) { .pid = ucred->pid, @@ -94,13 +112,5 @@ int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pi *ret_pidref = PIDREF_MAKE_FROM_PID(ucred->pid); } - if (ret_text) { - buf[n] = 0; - *ret_text = TAKE_PTR(buf); - } - - if (ret_ucred) - *ret_ucred = *ucred; - return 0; } diff --git a/src/sysupdate/sysupdate-transfer.c b/src/sysupdate/sysupdate-transfer.c index f3dea04314..932bf83076 100644 --- a/src/sysupdate/sysupdate-transfer.c +++ b/src/sysupdate/sysupdate-transfer.c @@ -989,7 +989,7 @@ static int helper_on_notify(sd_event_source *s, int fd, uint32_t revents, void * if (r == -EAGAIN) return 0; if (r < 0) - return log_warning_errno(r, "Failed to receive notification message: %m"); + return r; if (!pidref_equal(&ctx->pid, &sender_pid)) { log_warning("Got notification datagram from unexpected peer, ignoring."); diff --git a/src/sysupdate/sysupdated.c b/src/sysupdate/sysupdated.c index 1083345374..db11c30ac4 100644 --- a/src/sysupdate/sysupdated.c +++ b/src/sysupdate/sysupdated.c @@ -1657,7 +1657,7 @@ static int manager_on_notify(sd_event_source *s, int fd, uint32_t revents, void if (r == -EAGAIN) return 0; if (r < 0) - return log_warning_errno(r, "Failed to receive notification message: %m"); + return r; Job *j; HASHMAP_FOREACH(j, m->jobs) { From 4a5b06c5d1d3f4e902b8e5999a0c59aaf575e182 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 26 Feb 2025 00:26:57 +0100 Subject: [PATCH 2/3] shared/async: introduce asynchronous_close_many() helper --- src/shared/async.c | 7 +++++++ src/shared/async.h | 5 ++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/shared/async.c b/src/shared/async.c index e0c7e7dcaa..acde1b11d7 100644 --- a/src/shared/async.c +++ b/src/shared/async.c @@ -133,6 +133,13 @@ int asynchronous_close(int fd) { return -EBADF; /* return an invalidated fd */ } +void asynchronous_close_many(const int fds[], size_t n_fds) { + assert(fds || n_fds == 0); + + FOREACH_ARRAY(i, fds, n_fds) + asynchronous_close(*i); +} + int asynchronous_rm_rf(const char *p, RemoveFlags flags) { int r; diff --git a/src/shared/async.h b/src/shared/async.h index 2f5bbd51a5..d8604f8746 100644 --- a/src/shared/async.h +++ b/src/shared/async.h @@ -21,7 +21,10 @@ int asynchronous_sync(pid_t *ret_pid); int asynchronous_fsync(int fd, pid_t *ret_pid); + int asynchronous_close(int fd); -int asynchronous_rm_rf(const char *p, RemoveFlags flags); +void asynchronous_close_many(const int fds[], size_t n_fds); DEFINE_TRIVIAL_CLEANUP_FUNC(int, asynchronous_close); + +int asynchronous_rm_rf(const char *p, RemoveFlags flags); From 5d09689b5c336a9dbb3636a6f7e97ff017b068d3 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 19 Feb 2025 23:06:38 +0100 Subject: [PATCH 3/3] core/manager: port to notify_recv_with_fds() --- src/core/manager.c | 118 +++++---------------------------------- src/shared/notify-recv.c | 38 +++++++++++-- src/shared/notify-recv.h | 12 +++- 3 files changed, 56 insertions(+), 112 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index c5fd03e42b..90d741e563 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -68,6 +68,7 @@ #include "memory-util.h" #include "mkdir-label.h" #include "mount-util.h" +#include "notify-recv.h" #include "os-util.h" #include "parse-util.h" #include "path-lookup.h" @@ -2795,20 +2796,10 @@ static int manager_get_units_for_pidref(Manager *m, const PidRef *pidref, Unit * static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t revents, void *userdata) { Manager *m = ASSERT_PTR(userdata); - char buf[NOTIFY_BUFFER_MAX+1]; - struct iovec iovec = { - .iov_base = buf, - .iov_len = sizeof(buf)-1, - }; - CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred)) + CMSG_SPACE(sizeof(int)) /* SCM_PIDFD */ + - CMSG_SPACE(sizeof(int) * NOTIFY_FD_MAX)) control; - struct msghdr msghdr = { - .msg_iov = &iovec, - .msg_iovlen = 1, - .msg_control = &control, - .msg_controllen = sizeof(control), - }; - ssize_t n; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + struct ucred ucred; + _cleanup_free_ char *buf = NULL; + _cleanup_(fdset_free_asyncp) FDSet *fds = NULL; int r; assert(m->notify_fd == fd); @@ -2818,98 +2809,15 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - n = recvmsg_safe(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC); - if (ERRNO_IS_NEG_TRANSIENT(n)) - return 0; /* Spurious wakeup, try again */ - if (n == -ECHRNG) { - log_warning_errno(n, "Got message with truncated control data (too many fds sent?), ignoring."); + r = notify_recv_with_fds(m->notify_fd, &buf, &ucred, &pidref, &fds); + if (r == -EAGAIN) return 0; - } - if (n == -EXFULL) { - log_warning_errno(n, "Got message with truncated payload data, ignoring."); - return 0; - } - if (n < 0) + if (r < 0) /* If this is any other, real error, then stop processing this socket. This of course means * we won't take notification messages anymore, but that's still better than busy looping: * being woken up over and over again, but being unable to actually read the message from the * socket. */ - return log_error_errno(n, "Failed to receive notification message: %m"); - - _cleanup_close_ int pidfd = -EBADF; - struct ucred *ucred = NULL; - int *fd_array = NULL; - size_t n_fds = 0; - - struct cmsghdr *cmsg; - CMSG_FOREACH(cmsg, &msghdr) { - if (cmsg->cmsg_level != SOL_SOCKET) - continue; - - if (cmsg->cmsg_type == SCM_RIGHTS) { - assert(!fd_array); - fd_array = CMSG_TYPED_DATA(cmsg, int); - n_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); - } else if (cmsg->cmsg_type == SCM_CREDENTIALS && - cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) { - - assert(!ucred); - ucred = CMSG_TYPED_DATA(cmsg, struct ucred); - } else if (cmsg->cmsg_type == SCM_PIDFD) { - assert(pidfd < 0); - pidfd = *CMSG_TYPED_DATA(cmsg, int); - } - } - - _cleanup_(fdset_free_asyncp) FDSet *fds = NULL; - - if (n_fds > 0) { - assert(fd_array); - - r = fdset_new_array(&fds, fd_array, n_fds); - if (r < 0) { - close_many(fd_array, n_fds); - log_oom_warning(); - return 0; - } - } - - if (!ucred || !pid_is_valid(ucred->pid)) { - log_warning("Received notify message without valid credentials. Ignoring."); - return 0; - } - - _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - - if (pidfd >= 0) - r = pidref_set_pidfd_consume(&pidref, TAKE_FD(pidfd)); - else - r = pidref_set_pid(&pidref, ucred->pid); - if (r < 0) { - if (r == -ESRCH) - log_debug_errno(r, "Notify sender died before message is processed. Ignoring."); - else - log_warning_errno(r, "Failed to pin notify sender, ignoring message: %m"); - return 0; - } - - if (pidref.pid != ucred->pid) { - assert(pidref.fd >= 0); - - log_warning("Got SCM_PIDFD for process " PID_FMT " but SCM_CREDENTIALS for process " PID_FMT ". Ignoring.", - pidref.pid, ucred->pid); - return 0; - } - - /* As extra safety check, let's make sure the string we get doesn't contain embedded NUL bytes. - * We permit one trailing NUL byte in the message, but don't expect it. */ - if (n > 1 && memchr(buf, 0, n-1)) { - log_warning("Received notify message with embedded NUL bytes. Ignoring."); - return 0; - } - - /* Make sure it's NUL-terminated, then parse it to obtain the tags list. */ - buf[n] = 0; + return r; _cleanup_strv_free_ char **tags = strv_split_newlines(buf); if (!tags) { @@ -2919,7 +2827,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t /* Possibly a barrier fd, let's see. */ if (manager_process_barrier_fd(tags, fds)) { - log_debug("Received barrier notification message from PID " PID_FMT ".", ucred->pid); + log_debug("Received barrier notification message from PID " PID_FMT ".", pidref.pid); return 0; } @@ -2931,16 +2839,16 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t int n_array = manager_get_units_for_pidref(m, &pidref, &array); if (n_array < 0) { - log_warning_errno(n_array, "Failed to determine units for PID " PID_FMT ", ignoring: %m", ucred->pid); + log_warning_errno(n_array, "Failed to determine units for PID " PID_FMT ", ignoring: %m", pidref.pid); return 0; } if (n_array == 0) - log_debug("Cannot find unit for notify message of PID "PID_FMT", ignoring.", ucred->pid); + log_debug("Cannot find unit for notify message of PID "PID_FMT", ignoring.", pidref.pid); else /* And now invoke the per-unit callbacks. Note that manager_invoke_notify_message() will handle * duplicate units – making sure we only invoke each unit's handler once. */ FOREACH_ARRAY(u, array, n_array) - manager_invoke_notify_message(m, *u, &pidref, ucred, tags, fds); + manager_invoke_notify_message(m, *u, &pidref, &ucred, tags, fds); if (!fdset_isempty(fds)) log_warning("Got extra auxiliary fds with notification message, closing them."); diff --git a/src/shared/notify-recv.c b/src/shared/notify-recv.c index 8730e88080..ce12ec8e01 100644 --- a/src/shared/notify-recv.c +++ b/src/shared/notify-recv.c @@ -1,14 +1,16 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "async.h" #include "fd-util.h" #include "notify-recv.h" #include "socket-util.h" -int notify_recv( +int notify_recv_with_fds( int fd, char **ret_text, struct ucred *ret_ucred, - PidRef *ret_pidref) { + PidRef *ret_pidref, + FDSet **ret_fds) { char buf[NOTIFY_BUFFER_MAX]; struct iovec iovec = { @@ -38,7 +40,8 @@ int notify_recv( if (ERRNO_IS_NEG_TRANSIENT(n)) return -EAGAIN; if (n == -ECHRNG) { - log_warning_errno(n, "Got message with truncated control data (unexpected fds sent?), ignoring."); + log_warning_errno(n, "Got message with truncated control data (%s fds sent?), ignoring.", + ret_fds ? "too many" : "unexpected"); return -EAGAIN; } if (n == -EXFULL) { @@ -50,6 +53,8 @@ int notify_recv( const struct ucred *ucred = NULL; _cleanup_close_ int pidfd = -EBADF; + int *fd_array = NULL; + size_t n_fds = 0; struct cmsghdr *cmsg; CMSG_FOREACH(cmsg, &msghdr) { @@ -59,9 +64,10 @@ int notify_recv( switch (cmsg->cmsg_type) { case SCM_RIGHTS: - /* For now, just close every fd */ - close_many(CMSG_TYPED_DATA(cmsg, int), - (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int)); + assert(!fd_array && n_fds == 0); + + fd_array = CMSG_TYPED_DATA(cmsg, int); + n_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); break; case SCM_PIDFD: @@ -80,6 +86,23 @@ int notify_recv( } } + _cleanup_(fdset_free_asyncp) FDSet *fds = NULL; + if (n_fds > 0) { + assert(fd_array); + + if (!ret_fds) { + log_debug("Received fds via notification while none is expected, closing all."); + asynchronous_close_many(fd_array, n_fds); + } else { + r = fdset_new_array(&fds, fd_array, n_fds); + if (r < 0) { + asynchronous_close_many(fd_array, n_fds); + log_warning_errno(r, "Failed to collect fds from notification, ignoring message: %m"); + return -EAGAIN; + } + } + } + if ((ret_ucred || ret_pidref) && (!ucred || !pid_is_valid(ucred->pid))) return log_warning_errno(SYNTHETIC_ERRNO(EAGAIN), "Got notification datagram lacking valid credential information, ignoring."); @@ -112,5 +135,8 @@ int notify_recv( *ret_pidref = PIDREF_MAKE_FROM_PID(ucred->pid); } + if (ret_fds) + *ret_fds = TAKE_PTR(fds); + return 0; } diff --git a/src/shared/notify-recv.h b/src/shared/notify-recv.h index 5cd5062b35..0a38fd28b4 100644 --- a/src/shared/notify-recv.h +++ b/src/shared/notify-recv.h @@ -3,6 +3,16 @@ #include +#include "fdset.h" #include "pidref.h" -int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pidref); +int notify_recv_with_fds( + int fd, + char **ret_text, + struct ucred *ret_ucred, + PidRef *ret_pidref, + FDSet **ret_fds); + +static inline int notify_recv(int fd, char **ret_text, struct ucred *ret_ucred, PidRef *ret_pidref) { + return notify_recv_with_fds(fd, ret_text, ret_ucred, ret_pidref, NULL); +}