From 5a7ba74c1959691d79580a1c3f4d94bca94bab8e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Mon, 1 Jun 2020 19:00:30 -0700 Subject: [PATCH] Avoid calling `poll_oneoff` with zero subscriptions. (#162) * Avoid calling `poll_oneoff` with zero subscriptions. With https://github.com/WebAssembly/WASI/pull/193 merged, WASI is moving to make `poll_oneoff` with no arguments an error. Even though that's in ephemeral and not yet in a snapshot, we can start to anticipate it in libc: - Remove the `pause` function, since WASI has no signals and thus no way to ever wake it up short of having the host terminate it. - Make `poll` and `pselect` return `ENOTSUP` in the case of having no events to wait for. * Remove `pause` from the defined-symbols.txt list. * Fix __wasilibc_unmodified_upstream markers. * Check for zero subscriptions, rather than zero events. Make `poll` and `pselect` return `ENOTSUP` when asked to poll on zero subscriptions, rather than when the systerm returns zero events. While here, drop the `__wasilibc_unmodified_upstream` markers, which were already pretty noisy here, and would be significantly worse with this change. * Add comments about the subtle relationship between nfds and nsubscriptions. * Rewrite the comment. * Fix code quotes. --- expected/wasm32-wasi/defined-symbols.txt | 1 - .../cloudlibc/src/libc/poll/poll.c | 39 ++++++++++--------- .../cloudlibc/src/libc/sys/select/pselect.c | 39 ++++++++++--------- libc-bottom-half/sources/pause.c | 14 ------- libc-top-half/musl/include/unistd.h | 2 + 5 files changed, 42 insertions(+), 53 deletions(-) delete mode 100644 libc-bottom-half/sources/pause.c diff --git a/expected/wasm32-wasi/defined-symbols.txt b/expected/wasm32-wasi/defined-symbols.txt index 448551862..990d1391a 100644 --- a/expected/wasm32-wasi/defined-symbols.txt +++ b/expected/wasm32-wasi/defined-symbols.txt @@ -805,7 +805,6 @@ optind optopt optreset pathconf -pause perror poll posix_close diff --git a/libc-bottom-half/cloudlibc/src/libc/poll/poll.c b/libc-bottom-half/cloudlibc/src/libc/poll/poll.c index be76ecd7c..cde4e81c2 100644 --- a/libc-bottom-half/cloudlibc/src/libc/poll/poll.c +++ b/libc-bottom-half/cloudlibc/src/libc/poll/poll.c @@ -11,14 +11,14 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) { // Construct events for poll(). size_t maxevents = 2 * nfds + 1; __wasi_subscription_t subscriptions[maxevents]; - size_t nevents = 0; + size_t nsubscriptions = 0; for (size_t i = 0; i < nfds; ++i) { struct pollfd *pollfd = &fds[i]; if (pollfd->fd < 0) continue; bool created_events = false; if ((pollfd->events & POLLRDNORM) != 0) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .userdata = (uintptr_t)pollfd, .u.tag = __WASI_EVENTTYPE_FD_READ, @@ -27,7 +27,7 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) { created_events = true; } if ((pollfd->events & POLLWRNORM) != 0) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .userdata = (uintptr_t)pollfd, .u.tag = __WASI_EVENTTYPE_FD_WRITE, @@ -47,7 +47,7 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) { // Create extra event for the timeout. if (timeout >= 0) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .u.tag = __WASI_EVENTTYPE_CLOCK, .u.u.clock.id = __WASI_CLOCKID_REALTIME, @@ -56,15 +56,24 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) { } // Execute poll(). - __wasi_event_t events[nevents]; + size_t nevents; + __wasi_event_t events[nsubscriptions]; __wasi_errno_t error = -#ifdef __wasilibc_unmodified_upstream - __wasi_poll(subscriptions, events, nevents, &nevents); -#else - __wasi_poll_oneoff(subscriptions, events, nevents, &nevents); -#endif + __wasi_poll_oneoff(subscriptions, events, nsubscriptions, &nevents); if (error != 0) { - errno = error; + // WASI's poll requires at least one subscription, or else it returns + // `EINVAL`. Since a `poll` with nothing to wait for is valid in POSIX, + // return `ENOTSUP` to indicate that we don't support that case. + // + // Wasm has no signal handling, so if none of the user-provided `pollfd` + // elements, nor the timeout, led us to producing even one subscription + // to wait for, there would be no way for the poll to wake up. WASI + // returns `EINVAL` in this case, but for users of `poll`, `ENOTSUP` is + // more likely to be understood. + if (nsubscriptions == 0) + errno = ENOTSUP; + else + errno = error; return -1; } @@ -80,18 +89,10 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) { if (event->type == __WASI_EVENTTYPE_FD_READ || event->type == __WASI_EVENTTYPE_FD_WRITE) { struct pollfd *pollfd = (struct pollfd *)(uintptr_t)event->userdata; -#ifdef __wasilibc_unmodified_upstream // generated constant names - if (event->error == __WASI_EBADF) { -#else if (event->error == __WASI_ERRNO_BADF) { -#endif // Invalid file descriptor. pollfd->revents |= POLLNVAL; -#ifdef __wasilibc_unmodified_upstream // generated constant names - } else if (event->error == __WASI_EPIPE) { -#else } else if (event->error == __WASI_ERRNO_PIPE) { -#endif // Hangup on write side of pipe. pollfd->revents |= POLLHUP; } else if (event->error != 0) { diff --git a/libc-bottom-half/cloudlibc/src/libc/sys/select/pselect.c b/libc-bottom-half/cloudlibc/src/libc/sys/select/pselect.c index bd1d2fdc0..fdc470ea8 100644 --- a/libc-bottom-half/cloudlibc/src/libc/sys/select/pselect.c +++ b/libc-bottom-half/cloudlibc/src/libc/sys/select/pselect.c @@ -11,11 +11,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, fd_set *restrict errorfds, const struct timespec *restrict timeout, -#ifdef __wasilibc_unmodified_upstream - ...) { -#else const sigset_t *sigmask) { -#endif // Negative file descriptor upperbound. if (nfds < 0) { errno = EINVAL; @@ -40,13 +36,13 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, // Determine the maximum number of events. size_t maxevents = readfds->__nfds + writefds->__nfds + 1; __wasi_subscription_t subscriptions[maxevents]; - size_t nevents = 0; + size_t nsubscriptions = 0; // Convert the readfds set. for (size_t i = 0; i < readfds->__nfds; ++i) { int fd = readfds->__fds[i]; if (fd < nfds) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .userdata = fd, .u.tag = __WASI_EVENTTYPE_FD_READ, @@ -59,7 +55,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, for (size_t i = 0; i < writefds->__nfds; ++i) { int fd = writefds->__fds[i]; if (fd < nfds) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .userdata = fd, .u.tag = __WASI_EVENTTYPE_FD_WRITE, @@ -70,7 +66,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, // Create extra event for the timeout. if (timeout != NULL) { - __wasi_subscription_t *subscription = &subscriptions[nevents++]; + __wasi_subscription_t *subscription = &subscriptions[nsubscriptions++]; *subscription = (__wasi_subscription_t){ .u.tag = __WASI_EVENTTYPE_CLOCK, .u.u.clock.id = __WASI_CLOCKID_REALTIME, @@ -82,15 +78,24 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, } // Execute poll(). - __wasi_event_t events[nevents]; + size_t nevents; + __wasi_event_t events[nsubscriptions]; __wasi_errno_t error = -#ifdef __wasilibc_unmodified_upstream - __wasi_poll(subscriptions, events, nevents, &nevents); -#else - __wasi_poll_oneoff(subscriptions, events, nevents, &nevents); -#endif + __wasi_poll_oneoff(subscriptions, events, nsubscriptions, &nevents); if (error != 0) { - errno = error; + // WASI's poll requires at least one subscription, or else it returns + // `EINVAL`. Since a `pselect` with nothing to wait for is valid in POSIX, + // return `ENOTSUP` to indicate that we don't support that case. + // + // Wasm has no signal handling, so if none of the user-provided `pollfd` + // elements, nor the timeout, led us to producing even one subscription + // to wait for, there would be no way for the poll to wake up. WASI + // returns `EINVAL` in this case, but for users of `poll`, `ENOTSUP` is + // more likely to be understood. + if (nsubscriptions == 0) + errno = ENOTSUP; + else + errno = error; return -1; } @@ -99,11 +104,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds, const __wasi_event_t *event = &events[i]; if ((event->type == __WASI_EVENTTYPE_FD_READ || event->type == __WASI_EVENTTYPE_FD_WRITE) && -#ifdef __wasilibc_unmodified_upstream // generated constant names - event->error == __WASI_EBADF) { -#else event->error == __WASI_ERRNO_BADF) { -#endif errno = EBADF; return -1; } diff --git a/libc-bottom-half/sources/pause.c b/libc-bottom-half/sources/pause.c deleted file mode 100644 index 0e3e71250..000000000 --- a/libc-bottom-half/sources/pause.c +++ /dev/null @@ -1,14 +0,0 @@ -#include -#include -#include -#include - -int pause(void) { - size_t n; - __wasi_errno_t error = __wasi_poll_oneoff(0, 0, 0, &n); - if (error != 0) { - errno = error; - return -1; - } - __builtin_trap(); -} diff --git a/libc-top-half/musl/include/unistd.h b/libc-top-half/musl/include/unistd.h index c6fe070bb..e55f638e9 100644 --- a/libc-top-half/musl/include/unistd.h +++ b/libc-top-half/musl/include/unistd.h @@ -128,7 +128,9 @@ char *getcwd(char *, size_t); unsigned alarm(unsigned); #endif unsigned sleep(unsigned); +#ifdef __wasilibc_unmodified_upstream /* WASI has no pause */ int pause(void); +#endif #ifdef __wasilibc_unmodified_upstream /* WASI has no fork/exec */ pid_t fork(void);