unix: fix partial read handling after POLLHUP (#4997)

It was reported that PTYs on Linux sometimes report POLLHUP, return a
partial read, but still return more data on the next read.

Libuv contains an optimization where it assumes a partial read after
POLLHUP means the next read can be skipped because it's going to fail
with EOF anyway. That assumption was thought to be always true but,
alas, it isn't.

The fact the optimization has been present for 13 years and this is the
first bug report about it, indicates how rare this particular condition
is, but of course we can't skim on correctness.

The reworked optimization only uses POLLHUP as an input signal when
POLLIN is not also set. That means we no longer have to track partial
reads because we're going to try and read anyway as long as POLLIN is
set. It seems to cause no measurable regressions on the test suite or
the (lightly tested) benchmarks.

Fixes: https://github.com/libuv/libuv/issues/4992

src/unix/stream.c

@@ -1030,8 +1030,6 @@ static void uv__read(uv_stream_t* stream) {
   int err;
   int is_ipc;
 
-  stream->flags &= ~UV_HANDLE_READ_PARTIAL;
-
   /* Prevent loop starvation when the data comes in as fast as (or faster than)
    * we can read it. XXX Need to rearm fd if we switch to edge-triggered I/O.
    */
@@ -1146,14 +1144,18 @@ static void uv__read(uv_stream_t* stream) {
 #endif
       stream->read_cb(stream, nread, &buf);
 
-      /* Return if we didn't fill the buffer, there is no more data to read. */
-      if (nread < buflen) {
-        stream->flags |= UV_HANDLE_READ_PARTIAL;
+      /* Save a system call and return if we didn't fill the buffer
+       * completely, on the assumption the next read() will fail with EOF.
+       *
+       * Devices like PTYs sometimes operate in a packet-like mode where
+       * they don't return all available data in a single read but we'll
+       * catch it on the next read because of level-triggered I/O.
+       */
+      if (nread < buflen)
         return;
     }
   }
 }
-}
 
 
 int uv_shutdown(uv_shutdown_t* req, uv_stream_t* stream, uv_shutdown_cb cb) {
@@ -1202,22 +1204,23 @@ void uv__stream_io(uv_loop_t* loop, uv__io_t* w, unsigned int events) {
 
   assert(uv__stream_fd(stream) >= 0);
 
-  /* Ignore POLLHUP here. Even if it's set, there may still be data to read. */
-  if (events & (POLLIN | POLLERR | POLLHUP))
+  if (events & (POLLIN | POLLERR))
     uv__read(stream);
 
   if (uv__stream_fd(stream) == -1)
     return;  /* read_cb closed stream. */
 
   /* Short-circuit iff POLLHUP is set, the user is still interested in read
-   * events and uv__read() reported a partial read but not EOF. If the EOF
-   * flag is set, uv__read() called read_cb with err=UV_EOF and we don't
-   * have to do anything. If the partial read flag is not set, we can't
-   * report the EOF yet because there is still data to read.
+   * events and uv__read() didn't see EOF. If the EOF flag is set, uv__read()
+   * called read_cb with err=UV_EOF and we don't have to do anything.
+   *
+   * POLLIN should not be set because, at least on Linux and possibly other
+   * operating systems, devices like PTYs sometimes produce partial reads even
+   * when more data is available.
    */
   if ((events & POLLHUP) &&
+      !(events & POLLIN) &&
       (stream->flags & UV_HANDLE_READING) &&
-      (stream->flags & UV_HANDLE_READ_PARTIAL) &&
       !(stream->flags & UV_HANDLE_READ_EOF)) {
     uv_buf_t buf = { NULL, 0 };
     uv__stream_eof(stream, &buf);

src/uv-common.h

@@ -90,7 +90,6 @@ enum {
   UV_HANDLE_LISTENING                   = 0x00000040,
   UV_HANDLE_CONNECTION                  = 0x00000080,
   UV_HANDLE_SHUT                        = 0x00000200,
-  UV_HANDLE_READ_PARTIAL                = 0x00000400,
   UV_HANDLE_READ_EOF                    = 0x00000800,
 
   /* Used by streams and UDP handles. */

test/test-list.h

@@ -77,6 +77,7 @@ TEST_DECLARE   (tty_escape_sequence_processing)
 #endif
 TEST_DECLARE   (tty_file)
 TEST_DECLARE   (tty_pty)
+TEST_DECLARE   (tty_pty_partial)
 TEST_DECLARE   (stdio_over_pipes)
 TEST_DECLARE   (stdio_emulate_iocp)
 TEST_DECLARE   (ip6_pton)
@@ -665,6 +666,7 @@ TASK_LIST_START
 #endif
   TEST_ENTRY  (tty_file)
   TEST_ENTRY  (tty_pty)
+  TEST_ENTRY  (tty_pty_partial)
   TEST_ENTRY  (stdio_over_pipes)
   TEST_ENTRY  (stdio_emulate_iocp)
   TEST_ENTRY  (ip6_pton)

test/test-tty.c

@@ -460,3 +460,76 @@ TEST_IMPL(tty_pty) {
 #endif
   return 0;
 }
+
+#if !defined(__ANDROID__) && !defined(_WIN32)
+static int tty_pty_partial_read_count;
+
+static void tty_pty_partial_feeder(void *arg) {
+  static char buf[1<<13];
+  ssize_t n;
+  ssize_t r;
+  int fd;
+  int i;
+
+  fd = *(int *)arg;
+  memset(buf, 'x', sizeof(buf));
+  for (i = 0; i < 8; i++) {
+    for (n = 0; n < (int) sizeof(buf); n += r) {
+      do
+        r = write(fd, &buf[n], sizeof(buf) - n);
+      while (r == -1 && errno == EINTR);
+      ASSERT_GT(r, 0);
+    }
+  }
+  ASSERT_OK(close(fd));
+}
+
+static void tty_pty_partial_alloc_cb(uv_handle_t* handle,
+                                     size_t suggested_size,
+                                     uv_buf_t *buf) {
+  static char slab[1<<16];
+  *buf = uv_buf_init(slab, sizeof(slab));
+}
+
+static void tty_pty_partial_read_cb(uv_stream_t* stream,
+                                    ssize_t nread,
+                                    const uv_buf_t *buf) {
+  if (nread > 0)
+    tty_pty_partial_read_count += nread;
+  else
+    uv_close((uv_handle_t*) stream, NULL);
+}
+#endif  /* !defined(__ANDROID__) && !defined(_WIN32) */
+
+TEST_IMPL(tty_pty_partial) {
+#if !defined(__ANDROID__) && !defined(_WIN32)
+  int master_fd, slave_fd;
+  uv_tty_t master_tty;
+  uv_thread_t tid;
+  uv_loop_t loop;
+  int i;
+
+  /* This test is not 100% deterministic. If the bug it is testing for is
+   * present, then it fails about 1 in 3 times, that's why it runs in a loop.
+   */
+  for (i = 0; i < 10; i++) {
+    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL))
+      RETURN_SKIP("No pty available, skipping.");
+
+    tty_pty_partial_read_count = 0;
+    ASSERT_OK(uv_loop_init(&loop));
+    ASSERT_OK(uv_tty_init(&loop, &master_tty, master_fd, 0));
+    ASSERT_OK(uv_read_start((uv_stream_t*) &master_tty,
+                            tty_pty_partial_alloc_cb,
+                            tty_pty_partial_read_cb));
+    ASSERT(uv_is_readable((uv_stream_t*) &master_tty));
+    ASSERT(uv_is_writable((uv_stream_t*) &master_tty));
+    ASSERT_OK(uv_thread_create(&tid, tty_pty_partial_feeder, &slave_fd));
+    ASSERT_OK(uv_run(&loop, UV_RUN_DEFAULT));
+    ASSERT_OK(uv_thread_join(&tid));
+    ASSERT_EQ(tty_pty_partial_read_count, 65536);
+    MAKE_VALGRIND_HAPPY(&loop);
+  }
+#endif
+  return 0;
+}