On Wed, 2016-10-05 at 18:04 +1100, Brian May wrote:
> Hello All,
>
> Just looking at this issue in Wheezy. Looks like it should be easy to
> patch, assuming we consider this deserving a security update - it
> requires local access.
>
> The code is a bit different in wheezy:
>
> if ((n = recvmsg(m->notify_watch.fd, &msghdr, MSG_DONTWAIT)) <= 0) {
> if (n >= 0)
> return -EIO;
>
> if (errno == EAGAIN || errno == EINTR)
> break;
>
> return -errno;
> }
>
> The condition n >= 0 is a bit confusing, as we already checked that n <=
> 0. So if this succeeds I think n == 0.
Right.
> The upstream patch is:
>
> diff --git a/src/core/manager.c b/src/core/manager.c
> index 43e231c..5704005 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -1716,10 +1716,14 @@ 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 (errno == EAGAIN || errno == EINTR)
> - return 0;
> + if (!IN_SET(errno, EAGAIN, EINTR))
> + log_error("Failed to receive notification message: %m");
> ·
> - return -errno;
> + /* 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 (n == 0) {
> log_debug("Got zero-length notification message. Ignoring.");
>
> [next hunk appears not relevant except it changes another return to a
> return 0]
>
> So guessing maybe something like:
>
> --- systemd-44.orig/src/manager.c
> +++ systemd-44/src/manager.c
> @@ -2030,13 +2030,18 @@
> msghdr.msg_controllen = sizeof(control);
>
> if ((n = recvmsg(m->notify_watch.fd, &msghdr, MSG_DONTWAIT)) <= 0) {
> + /* 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... */
The upstream patch doesn't change the n == 0 case and I don't think
this backport should either. The comment belongs at the bottom of this
block.
Upstream version 219 changed the above if-statement to check for n < 0,
but that doesn't seem to have been quite correct. The version in
unstable has these patches to handle n == 0 properly:
If-the-notification-message-length-is-0-ignore-the-messag.patch
pid1-process-zero-length-notification-messages-again.patch
> if (n >= 0)
> - return -EIO;
> + return 0;
>
> if (errno == EAGAIN || errno == EINTR)
> + log_error("Failed to receive notification message: %m");
> break;
Missing braces.
Ben.
> - return -errno;
> + return 0;
> }
>
> if (msghdr.msg_controllen < CMSG_LEN(sizeof(struct ucred)) ||
> @@ -2058,7 +2063,7 @@
> assert((size_t) n < sizeof(buf));
> buf[n] = 0;
> if (!(tags = strv_split(buf, "\n\r")))
> - return -ENOMEM;
> + return 0;
>
> log_debug("Got notification message for unit %s", u->id);
>
> I have not yet claimed systemd yet, if somebody else wants to claim it
> before I do, go ahead.
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.Attachment:
signature.asc
Description: This is a digitally signed message part