From dad28bffd67692d6c22e0de98ddb0d217c034ec5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Jun 2020 13:40:25 +0200 Subject: [PATCH] tree-wide: check POLLNVAL everywhere poll() sets POLLNVAL inside of the poll structures if an invalid fd is passed. So far we generally didn't check for that, thus not taking notice of the error. Given that this specific kind of error is generally indication of a programming error, and given that our code is embedded into our projects via NSS or because people link against our library, let's explicitly check for this and convert it to EBADF. (I ran into a busy loop because of this missing check when some of my test code accidentally closed an fd it shouldn't close, so this is a real thing) --- src/basic/io-util.c | 13 +++++++++++-- src/basic/socket-util.c | 3 +++ src/journal/journalctl.c | 5 ++++- src/libsystemd/sd-bus/bus-socket.c | 2 ++ src/libsystemd/sd-bus/sd-bus.c | 9 ++++++++- src/libsystemd/sd-daemon/sd-daemon.c | 2 ++ src/libsystemd/sd-netlink/sd-netlink.c | 16 +++++++++++----- src/libudev/libudev-monitor.c | 5 ++++- src/shared/ask-password-api.c | 19 +++++++++++++++++++ src/shared/barrier.c | 12 ++++++++---- src/shared/utmp-wtmp.c | 3 ++- src/shared/varlink.c | 15 ++++++++++++--- src/sleep/sleep.c | 3 +++ src/stdio-bridge/stdio-bridge.c | 18 ++++++++++-------- .../tty-ask-password-agent.c | 4 ++++ src/udev/udevadm-settle.c | 7 ++++++- src/userdb/userwork.c | 2 ++ 17 files changed, 111 insertions(+), 27 deletions(-) diff --git a/src/basic/io-util.c b/src/basic/io-util.c index c906fc0741..4b3d1c814f 100644 --- a/src/basic/io-util.c +++ b/src/basic/io-util.c @@ -33,10 +33,13 @@ int flush_fd(int fd) { continue; return -errno; - - } else if (r == 0) + } + if (r == 0) return count; + if (pollfd.revents & POLLNVAL) + return -EBADF; + l = read(fd, buf, sizeof(buf)); if (l < 0) { @@ -169,6 +172,9 @@ int pipe_eof(int fd) { if (r == 0) return 0; + if (pollfd.revents & POLLNVAL) + return -EBADF; + return pollfd.revents & POLLHUP; } @@ -188,6 +194,9 @@ int fd_wait_for_event(int fd, int event, usec_t t) { if (r == 0) return 0; + if (pollfd.revents & POLLNVAL) + return -EBADF; + return pollfd.revents; } diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 05dd7e7001..2c990047fc 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -1002,6 +1002,9 @@ int flush_accept(int fd) { if (r == 0) return 0; + if (pollfd.revents & POLLNVAL) + return -EBADF; + if (iteration >= MAX_FLUSH_ITERATIONS) return log_debug_errno(SYNTHETIC_ERRNO(EBUSY), "Failed to flush connections within " STRINGIFY(MAX_FLUSH_ITERATIONS) " iterations."); diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index a082995498..859f4bbd44 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -2093,10 +2093,13 @@ static int wait_for_change(sd_journal *j, int poll_fd) { return log_error_errno(errno, "Couldn't wait for journal event: %m"); } - if (pollfds[1].revents & (POLLHUP|POLLERR)) /* STDOUT has been closed? */ + if (pollfds[1].revents & (POLLHUP|POLLERR|POLLNVAL)) /* STDOUT has been closed? */ return log_debug_errno(SYNTHETIC_ERRNO(ECANCELED), "Standard output has been closed."); + if (pollfds[0].revents & POLLNVAL) + return log_debug_errno(SYNTHETIC_ERRNO(EBADF), "Change fd closed?"); + r = sd_journal_process(j); if (r < 0) return log_error_errno(r, "Failed to process journal events: %m"); diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index acc313d164..778f80377f 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -1275,6 +1275,8 @@ int bus_socket_process_opening(sd_bus *b) { r = poll(&p, 1, 0); if (r < 0) return -errno; + if (p.revents & POLLNVAL) + return -EBADF; if (!(p.revents & (POLLOUT|POLLERR|POLLHUP))) return 0; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index cd1a79a256..9de5e454a6 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -3121,8 +3121,15 @@ static int bus_poll(sd_bus *bus, bool need_more, uint64_t timeout_usec) { r = ppoll(p, n, m == USEC_INFINITY ? NULL : timespec_store(&ts, m), NULL); if (r < 0) return -errno; + if (r == 0) + return 0; - return r > 0 ? 1 : 0; + if (p[0].revents & POLLNVAL) + return -EBADF; + if (n >= 2 && (p[1].revents & POLLNVAL)) + return -EBADF; + + return 1; } _public_ int sd_bus_wait(sd_bus *bus, uint64_t timeout_usec) { diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 587a1f2595..0bc0fc16ae 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -577,6 +577,8 @@ _public_ int sd_notify_barrier(int unset_environment, uint64_t timeout) { return -errno; if (r == 0) return -ETIMEDOUT; + if (pfd.revents & POLLNVAL) + return -EBADF; return 1; } diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 5b7081089e..ddeb4a7992 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -462,7 +462,6 @@ static usec_t calc_elapse(uint64_t usec) { } static int rtnl_poll(sd_netlink *rtnl, bool need_more, uint64_t timeout_usec) { - struct pollfd p[1] = {}; struct timespec ts; usec_t m = USEC_INFINITY; int r, e; @@ -495,14 +494,21 @@ static int rtnl_poll(sd_netlink *rtnl, bool need_more, uint64_t timeout_usec) { if (timeout_usec != (uint64_t) -1 && (m == (uint64_t) -1 || timeout_usec < m)) m = timeout_usec; - p[0].fd = rtnl->fd; - p[0].events = e; + struct pollfd p = { + .fd = rtnl->fd, + .events = e, + }; - r = ppoll(p, 1, m == (uint64_t) -1 ? NULL : timespec_store(&ts, m), NULL); + r = ppoll(&p, 1, m == (uint64_t) -1 ? NULL : timespec_store(&ts, m), NULL); if (r < 0) return -errno; + if (r == 0) + return 0; - return r > 0 ? 1 : 0; + if (p.revents & POLLNVAL) + return -EBADF; + + return 1; } int sd_netlink_wait(sd_netlink *nl, uint64_t timeout_usec) { diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c index 5f780e0be9..bf23021b53 100644 --- a/src/libudev/libudev-monitor.c +++ b/src/libudev/libudev-monitor.c @@ -216,8 +216,11 @@ static int udev_monitor_receive_sd_device(struct udev_monitor *udev_monitor, sd_ continue; return -errno; - } else if (r == 0) + } + if (r == 0) return -EAGAIN; + if (pfd.revents & POLLNVAL) + return -EBADF; /* receive next message */ break; diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 4c5781a11a..3d0b939106 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -305,6 +305,12 @@ int ask_password_plymouth( goto finish; } + if (pollfd[POLL_SOCKET].revents & POLLNVAL || + (notify >= 0 && pollfd[POLL_INOTIFY].revents & POLLNVAL)) { + r = -EBADF; + goto finish; + } + if (notify >= 0 && pollfd[POLL_INOTIFY].revents != 0) (void) flush_fd(notify); @@ -541,6 +547,12 @@ int ask_password_tty( goto finish; } + if ((pollfd[POLL_TTY].revents & POLLNVAL) || + (notify >= 0 && (pollfd[POLL_INOTIFY].revents & POLLNVAL))) { + r = -EBADF; + goto finish; + } + if (notify >= 0 && pollfd[POLL_INOTIFY].revents != 0 && keyname) { (void) flush_fd(notify); @@ -888,6 +900,13 @@ int ask_password_agent( goto finish; } + if (pollfd[FD_SOCKET].revents & POLLNVAL || + pollfd[FD_SIGNAL].revents & POLLNVAL || + (notify >= 0 && pollfd[FD_INOTIFY].revents & POLLNVAL)) { + r = -EBADF; + goto finish; + } + if (pollfd[FD_SIGNAL].revents & POLLIN) { r = -EINTR; goto finish; diff --git a/src/shared/barrier.c b/src/shared/barrier.c index bb5869dad4..80b597b5ca 100644 --- a/src/shared/barrier.c +++ b/src/shared/barrier.c @@ -218,10 +218,14 @@ static bool barrier_read(Barrier *b, int64_t comp) { uint64_t buf; int r; - r = poll(pfd, 2, -1); - if (r < 0 && IN_SET(errno, EAGAIN, EINTR)) - continue; - else if (r < 0) + r = poll(pfd, ELEMENTSOF(pfd), -1); + if (r < 0) { + if (IN_SET(errno, EAGAIN, EINTR)) + continue; + goto error; + } + if (pfd[0].revents & POLLNVAL || + pfd[1].revents & POLLNVAL) goto error; if (pfd[1].revents) { diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index fa4f32f353..7d0d90247c 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -334,9 +334,10 @@ static int write_to_terminal(const char *tty, const char *message) { k = poll(&pollfd, 1, (end - t) / USEC_PER_MSEC); if (k < 0) return -errno; - if (k == 0) return -ETIME; + if (pollfd.revents & POLLNVAL) + return -EBADF; n = write(fd, p, left); if (n < 0) { diff --git a/src/shared/varlink.c b/src/shared/varlink.c index ac111555e9..c0b5874225 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -1048,10 +1048,14 @@ int varlink_wait(Varlink *v, usec_t timeout) { NULL); if (r < 0) return -errno; + if (r == 0) + return 0; + + if (pfd.revents & POLLNVAL) + return -EBADF; handle_revents(v, pfd.revents); - - return r > 0 ? 1 : 0; + return 1; } int varlink_get_fd(Varlink *v) { @@ -1139,9 +1143,14 @@ int varlink_flush(Varlink *v) { .events = POLLOUT, }; - if (poll(&pfd, 1, -1) < 0) + r = poll(&pfd, 1, -1); + if (r < 0) return -errno; + assert(r > 0); + if (pfd.revents & POLLNVAL) + return -EBADF; + handle_revents(v, pfd.revents); } diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index fbfddc0262..4f9c23099a 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -271,6 +271,9 @@ static int execute_s2h(const SleepConfig *sleep_config) { tfd = safe_close(tfd); + if (FLAGS_SET(fds.revents, POLLNVAL)) + return log_error_errno(SYNTHETIC_ERRNO(EBADF), "Invalid timer fd to sleep on?"); + if (!FLAGS_SET(fds.revents, POLLIN)) /* We woke up before the alarm time, we are done. */ return 0; diff --git a/src/stdio-bridge/stdio-bridge.c b/src/stdio-bridge/stdio-bridge.c index 52b9ce4555..ca145aebf9 100644 --- a/src/stdio-bridge/stdio-bridge.c +++ b/src/stdio-bridge/stdio-bridge.c @@ -238,17 +238,19 @@ static int run(int argc, char *argv[]) { ts = timespec_store(&_ts, t); } - { - struct pollfd p[3] = { - {.fd = fd, .events = events_a }, - {.fd = STDIN_FILENO, .events = events_b & POLLIN }, - {.fd = STDOUT_FILENO, .events = events_b & POLLOUT }, - }; + struct pollfd p[3] = { + { .fd = fd, .events = events_a }, + { .fd = STDIN_FILENO, .events = events_b & POLLIN }, + { .fd = STDOUT_FILENO, .events = events_b & POLLOUT }, + }; - r = ppoll(p, ELEMENTSOF(p), ts, NULL); - } + r = ppoll(p, ELEMENTSOF(p), ts, NULL); if (r < 0) return log_error_errno(errno, "ppoll() failed: %m"); + if (p[0].revents & POLLNVAL || + p[1].revents & POLLNVAL || + p[2].revents & POLLNVAL) + return log_error_errno(SYNTHETIC_ERRNO(EBADF), "Invalid file descriptor to poll on?"); } return 0; diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index eb84719111..4371da4785 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -395,6 +395,10 @@ static int process_and_watch_password_files(bool watch) { return -errno; } + if (pollfd[FD_SIGNAL].revents & POLLNVAL || + pollfd[FD_INOTIFY].revents & POLLNVAL) + return -EBADF; + if (pollfd[FD_INOTIFY].revents != 0) (void) flush_fd(notify); diff --git a/src/udev/udevadm-settle.c b/src/udev/udevadm-settle.c index 7d7044eac3..1fa267c831 100644 --- a/src/udev/udevadm-settle.c +++ b/src/udev/udevadm-settle.c @@ -202,7 +202,12 @@ int settle_main(int argc, char *argv[], void *userdata) { return -ETIMEDOUT; /* wake up when queue becomes empty */ - if (poll(&pfd, 1, MSEC_PER_SEC) > 0 && pfd.revents & POLLIN) { + r = poll(&pfd, 1, MSEC_PER_SEC); + if (r < 0) + return -errno; + if (pfd.revents & POLLNVAL) + return -EBADF; + if (r > 0 && pfd.revents & POLLIN) { r = udev_queue_flush(queue); if (r < 0) return log_error_errno(r, "Failed to flush queue: %m"); diff --git a/src/userdb/userwork.c b/src/userdb/userwork.c index ac04af0ca8..4a8c98384d 100644 --- a/src/userdb/userwork.c +++ b/src/userdb/userwork.c @@ -757,6 +757,8 @@ static int run(int argc, char *argv[]) { if (poll(&pfd, 1, 0) < 0) return log_error_errno(errno, "Failed to test for POLLIN on listening socket: %m"); + if (FLAGS_SET(pfd.revents, POLLNVAL)) + return log_error_errno(SYNTHETIC_ERRNO(EBADF), "Listening socket dead?"); if (FLAGS_SET(pfd.revents, POLLIN)) { pid_t parent;