From: Mike Yuan <me@yhndnzj.com>
Date: Fri, 21 Feb 2025 14:16:46 +0000 (+0100)
Subject: notify-recv: several followups
X-Git-Tag: v258-rc1~1250^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=74cd56d31b9673d2a3248e6ffbea2294a0fe2790;p=thirdparty%2Fsystemd.git

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.
---

diff --git a/src/import/importd.c b/src/import/importd.c
index 5a1dcb92a5d..21c6d969bca 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 a2c53005aa9..b736bf704fb 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 4ba3694280b..066156a5ca0 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 c3596d0258a..8730e88080c 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_pidref) {
-                assert(ucred);
-                assert(ucred->pid > 0);
+        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) {
                 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 f3dea04314b..932bf83076c 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 1083345374c..db11c30ac48 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) {