From c55ae51e77e1fc56fde7bc3466dd2021ff3856cb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Oct 2016 12:08:51 +0200 Subject: manager: don't ever busy loop when we get a notification message we can't process If the kernel doesn't permit us to dequeue/process an incoming notification datagram message it's still better to stop processing the notification messages altogether than to enter a busy loop where we keep getting notified but can't do a thing about it. With this change, manager_dispatch_notify_fd() behaviour is changed like this: - if an error indicating a spurious wake-up is seen on recvmsg(), ignore it (EAGAIN/EINTR) - if any other error is seen on recvmsg() propagate it, thus disabling processing of further wakeups - if any error is seen on later code in the function, warn about it but do not propagate it, as in this cas we're not going to busy loop as the offending message is already dequeued. --- src/core/manager.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'src/core') diff --git a/src/core/manager.c b/src/core/manager.c index 5253cb3712..ab65d630a1 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1722,14 +1722,13 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t n = recvmsg(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC); if (n < 0) { - if (!IN_SET(errno, EAGAIN, EINTR)) - log_error("Failed to receive notification message: %m"); + if (IN_SET(errno, EAGAIN, EINTR)) + return 0; /* Spurious wakeup, try again */ - /* It's not an option to return an error here since it - * would disable the notification handler entirely. Services - * wouldn't be able to send the WATCHDOG message for - * example... */ - return 0; + /* If this is any other, real error, then let's stop processing this socket. This of course means we + * won't take notification messages anymore, but that's still better than busy looping around this: + * being woken up over and over again but being unable to actually read the message off the socket. */ + return log_error_errno(errno, "Failed to receive notification message: %m"); } CMSG_FOREACH(cmsg, &msghdr) { -- cgit v1.2.3-54-g00ecf From 045a3d5989f7565dc496013a9e96d95d86a12cc8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Oct 2016 12:12:10 +0200 Subject: manager: be stricter with incomining notifications, warn properly about too large ones Let's make the kernel let us know the full, original datagram size of the incoming message. If it's larger than the buffer space provided by us, drop the whole message with a warning. Before this change the kernel would truncate the message for us to the buffer space provided, and we'd not complain about this, and simply process the incomplete message as far as it made sense. --- src/core/manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/manager.c b/src/core/manager.c index ab65d630a1..66b8904e4e 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1720,7 +1720,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - n = recvmsg(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC); + n = recvmsg(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC|MSG_TRUNC); if (n < 0) { if (IN_SET(errno, EAGAIN, EINTR)) return 0; /* Spurious wakeup, try again */ @@ -1761,7 +1761,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - if ((size_t) n >= sizeof(buf)) { + if ((size_t) n >= sizeof(buf) || (msghdr.msg_flags & MSG_TRUNC)) { log_warning("Received notify message exceeded maximum size. Ignoring."); return 0; } -- cgit v1.2.3-54-g00ecf From 875ca88da576b4f7c412f6a5e1fc642ba3bd288a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Oct 2016 12:14:33 +0200 Subject: manager: tighten incoming notification message checks Let's not accept datagrams with embedded NUL bytes. Previously we'd simply ignore everything after the first NUL byte. But given that sending us that is pretty ugly let's instead complain and refuse. With this change we'll only accept messages that have exactly zero or one NUL bytes at the very end of the datagram. --- src/core/manager.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/manager.c b/src/core/manager.c index 66b8904e4e..34db276a7d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1766,8 +1766,14 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - /* The message should be a string. Here we make sure it's NUL-terminated, - * but only the part until first NUL will be used anyway. */ + /* 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. */ buf[n] = 0; /* Notify every unit that might be interested, but try -- cgit v1.2.3-54-g00ecf From 8f4d6401351b1114117a1517283f00c7f349c9ff Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Fri, 7 Oct 2016 09:39:42 -0400 Subject: core: only warn on short reads on signal fd --- src/core/manager.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'src/core') diff --git a/src/core/manager.c b/src/core/manager.c index 34db276a7d..c1dce62a18 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1946,14 +1946,17 @@ static int manager_dispatch_signal_fd(sd_event_source *source, int fd, uint32_t for (;;) { n = read(m->signal_fd, &sfsi, sizeof(sfsi)); if (n != sizeof(sfsi)) { + if (n >= 0) { + log_warning("Truncated read from signal fd (%zu bytes)!", n); + return 0; + } - if (n >= 0) - return -EIO; - - if (errno == EINTR || errno == EAGAIN) + if (IN_SET(errno, EINTR, EAGAIN)) break; - return -errno; + /* We return an error here, which will kill this handler, + * to avoid a busy loop on read error. */ + return log_error_errno(errno, "Reading from signal fd failed: %m"); } log_received_signal(sfsi.ssi_signo == SIGCHLD || -- cgit v1.2.3-54-g00ecf