From 44e61dab7effb739f543b8b4eee43f577bf2db49 Mon Sep 17 00:00:00 2001 From: Andy Pan Date: Tue, 10 Sep 2024 02:22:15 +0800 Subject: [PATCH 1/2] kqueue: disallow ill-suited file descriptor kinds (#4513) Follows up on https://github.com/libuv/libuv/pull/659. Signed-off-by: Andy Pan --- src/unix/kqueue.c | 33 +++++++++++++++++++++++++++ test/test-poll.c | 57 +++++++++++++++++++++++++++++++++++++---------- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/src/unix/kqueue.c b/src/unix/kqueue.c index 1762165ee09..876b717086c 100644 --- a/src/unix/kqueue.c +++ b/src/unix/kqueue.c @@ -99,6 +99,39 @@ int uv__io_fork(uv_loop_t* loop) { int uv__io_check_fd(uv_loop_t* loop, int fd) { struct kevent ev; int rc; + struct stat sb; +#ifdef __APPLE__ + char path[MAXPATHLEN]; +#endif + + if (uv__fstat(fd, &sb)) + return UV__ERR(errno); + + /* On FreeBSD, kqueue only supports EVFILT_READ notification for regular files + * and always reports ready events for writing, resulting in busy-looping. + * + * On Darwin, DragonFlyBSD, NetBSD and OpenBSD, kqueue reports ready events for + * regular files as readable and writable only once, acting like an EV_ONESHOT. + * + * Neither of the above cases should be added to the kqueue. + */ + if (S_ISREG(sb.st_mode) || S_ISDIR(sb.st_mode)) + return UV_EINVAL; + +#ifdef __APPLE__ + /* On Darwin (both macOS and iOS), in addition to regular files, FIFOs also don't + * work properly with kqueue: the disconnection from the last writer won't trigger + * an event for kqueue in spite of what the man pages say. Thus, we also disallow + * the case of S_IFIFO. */ + if (S_ISFIFO(sb.st_mode)) { + /* File descriptors of FIFO, pipe and kqueue share the same type of file, + * therefore there is no way to tell them apart via stat.st_mode&S_IFMT. + * Fortunately, FIFO is the only one that has a persisted file on filesystem, + * from which we're able to make the distinction for it. */ + if (!fcntl(fd, F_GETPATH, path)) + return UV_EINVAL; + } +#endif rc = 0; EV_SET(&ev, fd, EVFILT_READ, EV_ADD, 0, 0, 0); diff --git a/test/test-poll.c b/test/test-poll.c index fcd644f2c25..5161de25376 100644 --- a/test/test-poll.c +++ b/test/test-poll.c @@ -626,26 +626,59 @@ TEST_IMPL(poll_unidirectional) { /* Windows won't let you open a directory so we open a file instead. - * OS X lets you poll a file so open the $PWD instead. Both fail - * on Linux so it doesn't matter which one we pick. Both succeed - * on FreeBSD, Solaris and AIX so skip the test on those platforms. + * OS X lets you poll a file so open the $PWD instead. Both fail + * on Linux so it doesn't matter which one we pick. Both succeed + * on Solaris and AIX so skip the test on those platforms. + * On *BSD/Darwin, we disallow polling of regular files, directories. + * In addition to regular files, we also disallow FIFOs on Darwin. */ +#ifdef __APPLE__ +#define TEST_POLL_FIFO_PATH "/tmp/uv-test-poll-fifo" +#endif TEST_IMPL(poll_bad_fdtype) { -#if !defined(__DragonFly__) && !defined(__FreeBSD__) && !defined(__sun) && \ +#if !defined(__sun) && \ !defined(_AIX) && !defined(__MVS__) && \ - !defined(__OpenBSD__) && !defined(__CYGWIN__) && !defined(__MSYS__) && \ - !defined(__NetBSD__) + !defined(__CYGWIN__) && !defined(__MSYS__) uv_poll_t poll_handle; - int fd; + int fd[2]; #if defined(_WIN32) - fd = _open("test/fixtures/empty_file", UV_FS_O_RDONLY); + fd[0] = _open("test/fixtures/empty_file", UV_FS_O_RDONLY); #else - fd = open(".", UV_FS_O_RDONLY); + fd[0] = open(".", UV_FS_O_RDONLY); +#endif + ASSERT_NE(fd[0], -1); + ASSERT_NE(0, uv_poll_init(uv_default_loop(), &poll_handle, fd[0])); + ASSERT_OK(close(fd[0])); +#if defined(__APPLE__) || \ + defined(__DragonFly__) || \ + defined(__FreeBSD__) || \ + defined(__OpenBSD__) || \ + defined(__NetBSD__) + fd[0] = open("test/fixtures/empty_file", UV_FS_O_RDONLY); + ASSERT_NE(fd[0], -1); + /* Regular files should be banned from kqueue. */ + ASSERT_NE(0, uv_poll_init(uv_default_loop(), &poll_handle, fd[0])); + ASSERT_OK(close(fd[0])); +#ifdef __APPLE__ + ASSERT_OK(pipe(fd)); + /* Pipes should be permitted in kqueue. */ + ASSERT_EQ(0, uv_poll_init(uv_default_loop(), &poll_handle, fd[0])); + ASSERT_OK(close(fd[0])); + ASSERT_OK(close(fd[1])); + + ASSERT_OK(mkfifo(TEST_POLL_FIFO_PATH, 0600)); + fd[0] = open(TEST_POLL_FIFO_PATH, O_RDONLY | O_NONBLOCK); + ASSERT_NE(fd[0], -1); + fd[1] = open(TEST_POLL_FIFO_PATH, O_WRONLY | O_NONBLOCK); + ASSERT_NE(fd[1], -1); + /* FIFOs should be banned from kqueue. */ + ASSERT_NE(0, uv_poll_init(uv_default_loop(), &poll_handle, fd[0])); + ASSERT_OK(close(fd[0])); + ASSERT_OK(close(fd[1])); + unlink(TEST_POLL_FIFO_PATH); +#endif #endif - ASSERT_NE(fd, -1); - ASSERT_NE(0, uv_poll_init(uv_default_loop(), &poll_handle, fd)); - ASSERT_OK(close(fd)); #endif MAKE_VALGRIND_HAPPY(uv_default_loop()); From a49f264dff6bc853d67e2d16f635ba8d80bc62a6 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 11 Sep 2024 10:33:54 +0200 Subject: [PATCH 2/2] unix: restore tty attributes on handle close (#4399) Libuv stores the `struct termios` for use inside uv_tty_reset_mode(). Node.js uses said function to restore the tty to its original mode on SIGINT or SIGTERM, when there is no opportunity to shut down the process normally. Track uv_tty_t handle closing, otherwise we might be trying to use a stale termios. The current solution is not ideal because there can be multiple handles that refer to the same tty/pty and, for various reasons, we can't really determine when we close the last handle. The last handle may not even be inside the current process. Still, all things considered, it's probably (hopefully!) an improvement over the status quo. Refs: https://github.com/libuv/libuv/issues/4398 --- src/unix/core.c | 2 +- src/unix/internal.h | 3 +++ src/unix/tty.c | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/unix/core.c b/src/unix/core.c index cfa65023f99..b74552efcf4 100644 --- a/src/unix/core.c +++ b/src/unix/core.c @@ -156,7 +156,7 @@ void uv_close(uv_handle_t* handle, uv_close_cb close_cb) { break; case UV_TTY: - uv__stream_close((uv_stream_t*)handle); + uv__tty_close((uv_tty_t*)handle); break; case UV_TCP: diff --git a/src/unix/internal.h b/src/unix/internal.h index a987be60321..86c0d18aacd 100644 --- a/src/unix/internal.h +++ b/src/unix/internal.h @@ -293,6 +293,9 @@ int uv__tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb); int uv__tcp_nodelay(int fd, int on); int uv__tcp_keepalive(int fd, int on, unsigned int delay); +/* tty */ +void uv__tty_close(uv_tty_t* handle); + /* pipe */ int uv__pipe_listen(uv_pipe_t* handle, int backlog, uv_connection_cb cb); diff --git a/src/unix/tty.c b/src/unix/tty.c index d099bdb3b67..793054ba5a9 100644 --- a/src/unix/tty.c +++ b/src/unix/tty.c @@ -335,6 +335,37 @@ int uv_tty_set_mode(uv_tty_t* tty, uv_tty_mode_t mode) { } +void uv__tty_close(uv_tty_t* handle) { + int expected; + int fd; + + fd = handle->io_watcher.fd; + if (fd == -1) + goto done; + + /* This is used for uv_tty_reset_mode() */ + do + expected = 0; + while (!atomic_compare_exchange_strong(&termios_spinlock, &expected, 1)); + + if (fd == orig_termios_fd) { + /* XXX(bnoordhuis) the tcsetattr is probably wrong when there are still + * other uv_tty_t handles active that refer to the same tty/pty but it's + * hard to recognize that particular situation without maintaining some + * kind of process-global data structure, and that still won't work in a + * multi-process setup. + */ + uv__tcsetattr(fd, TCSANOW, &orig_termios); + orig_termios_fd = -1; + } + + atomic_store(&termios_spinlock, 0); + +done: + uv__stream_close((uv_stream_t*) handle); +} + + int uv_tty_get_winsize(uv_tty_t* tty, int* width, int* height) { struct winsize ws; int err; @@ -452,7 +483,7 @@ int uv_tty_reset_mode(void) { saved_errno = errno; if (atomic_exchange(&termios_spinlock, 1)) - return UV_EBUSY; /* In uv_tty_set_mode(). */ + return UV_EBUSY; /* In uv_tty_set_mode() or uv__tty_close(). */ err = 0; if (orig_termios_fd != -1)