Revert "unix,fs: fix for potential partial reads/writes"

This commit has been reported as introducing a backwards-incompatible
change in reading from stdin and is independently suspected of breaking
the Node.js test suite on MacOS and maybe other platforms, possibly in
combination with commit fd049399 ("unix,tcp: avoid marking server
sockets connected".)

This reverts commit 14bfc27e64.

Fixes: https://github.com/libuv/libuv/issues/1716
Fixes: https://github.com/libuv/libuv/issues/1720
Fixes: https://github.com/nodejs/node/issues/18225
PR-URL: https://github.com/libuv/libuv/pull/1717
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

Makefile.am

@@ -165,6 +165,7 @@ test_run_tests_SOURCES = test/blackhole-server.c \
                          test/test-default-loop-close.c \
                          test/test-delayed-accept.c \
                          test/test-dlerror.c \
+                         test/test-eintr-handling.c \
                          test/test-embed.c \
                          test/test-emfile.c \
                          test/test-env-vars.c \

src/unix/fs.c

@@ -334,7 +334,25 @@ static ssize_t uv__fs_read(uv_fs_t* req) {
     if (no_preadv) retry:
 # endif
     {
-      result = pread(req->file, req->bufs[0].base, req->bufs[0].len, req->off);
+      off_t nread;
+      size_t index;
+
+      nread = 0;
+      index = 0;
+      result = 1;
+      do {
+        if (req->bufs[index].len > 0) {
+          result = pread(req->file,
+                         req->bufs[index].base,
+                         req->bufs[index].len,
+                         req->off + nread);
+          if (result > 0)
+            nread += result;
+        }
+        index++;
+      } while (index < req->nbufs && result > 0);
+      if (nread > 0)
+        result = nread;
     }
 # if defined(__linux__)
     else {
@@ -722,7 +740,25 @@ static ssize_t uv__fs_write(uv_fs_t* req) {
     if (no_pwritev) retry:
 # endif
     {
-      r = pwrite(req->file, req->bufs[0].base, req->bufs[0].len, req->off);
+      off_t written;
+      size_t index;
+
+      written = 0;
+      index = 0;
+      r = 0;
+      do {
+        if (req->bufs[index].len > 0) {
+          r = pwrite(req->file,
+                     req->bufs[index].base,
+                     req->bufs[index].len,
+                     req->off + written);
+          if (r > 0)
+            written += r;
+        }
+        index++;
+      } while (index < req->nbufs && r >= 0);
+      if (written > 0)
+        r = written;
     }
 # if defined(__linux__)
     else {
@@ -972,19 +1008,6 @@ static int uv__fs_fstat(int fd, uv_stat_t *buf) {
   return ret;
 }
 
-static size_t uv__fs_buf_offset(uv_buf_t* bufs, size_t size) {
-  size_t offset;
-  /* Figure out which bufs are done */
-  for (offset = 0; size > 0 && bufs[offset].len <= size; ++offset)
-    size -= bufs[offset].len;
-
-  /* Fix a partial read/write */
-  if (size > 0) {
-    bufs[offset].base += size;
-    bufs[offset].len -= size;
-  }
-  return offset;
-}
 
 typedef ssize_t (*uv__fs_buf_iter_processor)(uv_fs_t* req);
 static ssize_t uv__fs_buf_iter(uv_fs_t* req, uv__fs_buf_iter_processor process) {
@@ -1004,10 +1027,7 @@ static ssize_t uv__fs_buf_iter(uv_fs_t* req, uv__fs_buf_iter_processor process)
     if (req->nbufs > iovmax)
       req->nbufs = iovmax;
 
-    do
+    result = process(req);
-      result = process(req);
-    while (result < 0 && errno == EINTR);
-
     if (result <= 0) {
       if (total == 0)
         total = result;
@@ -1017,12 +1037,14 @@ static ssize_t uv__fs_buf_iter(uv_fs_t* req, uv__fs_buf_iter_processor process)
     if (req->off >= 0)
       req->off += result;
 
-    req->nbufs = uv__fs_buf_offset(req->bufs, result);
     req->bufs += req->nbufs;
     nbufs -= req->nbufs;
     total += result;
   }
 
+  if (errno == EINTR && total == -1)
+    return total;
+
   if (bufs != req->bufsml)
     uv__free(bufs);
 

test/test-eintr-handling.c

@@ -0,0 +1,94 @@
+/* Copyright libuv project contributors. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "uv.h"
+#include "task.h"
+
+#ifdef _WIN32
+
+TEST_IMPL(eintr_handling) {
+  RETURN_SKIP("Test not implemented on Windows.");
+}
+
+#else  /* !_WIN32 */
+
+#include <string.h>
+#include <unistd.h>
+
+static uv_loop_t* loop;
+static uv_fs_t read_req;
+static uv_buf_t iov;
+
+static char buf[32];
+static char test_buf[] = "test-buffer\n";
+int pipe_fds[2];
+
+struct thread_ctx {
+  uv_barrier_t barrier;
+  int fd;
+};
+
+static void thread_main(void* arg) {
+  int nwritten;
+  ASSERT(0 == kill(getpid(), SIGUSR1));
+
+  do
+    nwritten = write(pipe_fds[1], test_buf, sizeof(test_buf));
+  while (nwritten == -1 && errno == EINTR);
+
+  ASSERT(nwritten == sizeof(test_buf));
+}
+
+static void sig_func(uv_signal_t* handle, int signum) {
+  uv_signal_stop(handle);
+}
+
+TEST_IMPL(eintr_handling) {
+  struct thread_ctx ctx;
+  uv_thread_t thread;
+  uv_signal_t signal;
+  int nread;
+
+  iov = uv_buf_init(buf, sizeof(buf));
+  loop = uv_default_loop();
+
+  ASSERT(0 == uv_signal_init(loop, &signal));
+  ASSERT(0 == uv_signal_start(&signal, sig_func, SIGUSR1));
+
+  ASSERT(0 == pipe(pipe_fds));
+  ASSERT(0 == uv_thread_create(&thread, thread_main, &ctx));
+
+  nread = uv_fs_read(loop, &read_req, pipe_fds[0], &iov, 1, -1, NULL);
+
+  ASSERT(nread == sizeof(test_buf));
+  ASSERT(0 == strcmp(buf, test_buf));
+
+  ASSERT(0 == uv_run(loop, UV_RUN_DEFAULT));
+
+  ASSERT(0 == close(pipe_fds[0]));
+  ASSERT(0 == close(pipe_fds[1]));
+  uv_close((uv_handle_t*) &signal, NULL);
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
+
+#endif  /* !_WIN32 */

test/test-fs.c

@@ -2890,131 +2890,6 @@ TEST_IMPL(fs_write_alotof_bufs_with_offset) {
 }
 
 
-#ifdef _WIN32
-
-TEST_IMPL(fs_partial_read) {
-  RETURN_SKIP("Test not implemented on Windows.");
-}
-
-TEST_IMPL(fs_partial_write) {
-  RETURN_SKIP("Test not implemented on Windows.");
-}
-
-#else  /* !_WIN32 */
-
-static void thread_exec(int fd, char* data, int size, int interval, int doread) {
-  pid_t pid;
-  ssize_t result;
-
-  pid = getpid();
-  result = 1;
-
-  while (size > 0 && result > 0) {
-    do {
-      if (doread)
-        result = write(fd, data, size < interval ? size : interval);
-      else
-        result = read(fd, data, size < interval ? size : interval);
-    } while (result == -1 && errno == EINTR);
-
-    kill(pid, SIGUSR1);
-    size -= result;
-    data += result;
-  }
-
-  ASSERT(size == 0);
-  ASSERT(result > 0);
-}
-
-struct thread_ctx {
-  int fd;
-  char *data;
-  int size;
-  int interval;
-  int doread;
-};
-
-static void thread_main(void* arg) {
-  struct thread_ctx *ctx;
-  ctx = (struct thread_ctx*)arg;
-  thread_exec(ctx->fd, ctx->data, ctx->size, ctx->interval, ctx->doread);
-}
-
-static void sig_func(uv_signal_t* handle, int signum) {
-  uv_signal_stop(handle);
-}
-
-static void test_fs_partial(int doread) {
-  struct thread_ctx ctx;
-  uv_thread_t thread;
-  uv_signal_t signal;
-  int pipe_fds[2];
-  size_t iovcount;
-  uv_buf_t* iovs;
-  char* buffer;
-  size_t index;
-  int result;
-
-  iovcount = 54321;
-
-  iovs = malloc(sizeof(*iovs) * iovcount);
-  ASSERT(iovs != NULL);
-
-  ctx.doread = doread;
-  ctx.interval = 1000;
-  ctx.size = sizeof(test_buf) * iovcount;
-  ctx.data = malloc(ctx.size);
-  ASSERT(ctx.data != NULL);
-  buffer = malloc(ctx.size);
-  ASSERT(buffer != NULL);
-
-  for (index = 0; index < iovcount; ++index)
-    iovs[index] = uv_buf_init(buffer + index * sizeof(test_buf), sizeof(test_buf));
-
-  loop = uv_default_loop();
-
-  ASSERT(0 == uv_signal_init(loop, &signal));
-  ASSERT(0 == uv_signal_start(&signal, sig_func, SIGUSR1));
-
-  ASSERT(0 == pipe(pipe_fds));
-
-  ctx.fd = pipe_fds[doread];
-  ASSERT(0 == uv_thread_create(&thread, thread_main, &ctx));
-
-  if (doread)
-    result = uv_fs_read(loop, &read_req, pipe_fds[0], iovs, iovcount, -1, NULL);
-  else
-    result = uv_fs_write(loop, &write_req, pipe_fds[1], iovs, iovcount, -1, NULL);
-
-  ASSERT(result == ctx.size);
-  ASSERT(0 == memcmp(buffer, ctx.data, result));
-
-  ASSERT(0 == uv_thread_join(&thread));
-  ASSERT(0 == uv_run(loop, UV_RUN_DEFAULT));
-
-  ASSERT(0 == close(pipe_fds[0]));
-  ASSERT(0 == close(pipe_fds[1]));
-  uv_close((uv_handle_t*) &signal, NULL);
-
-  free(iovs);
-  free(buffer);
-  free(ctx.data);
-
-  MAKE_VALGRIND_HAPPY();
-}
-
-TEST_IMPL(fs_partial_read) {
-  test_fs_partial(1);
-  return 0;
-}
-
-TEST_IMPL(fs_partial_write) {
-  test_fs_partial(0);
-  return 0;
-}
-
-#endif/* _WIN32 */
-
 TEST_IMPL(fs_read_write_null_arguments) {
   int r;
 
@@ -3223,7 +3098,7 @@ TEST_IMPL(fs_exclusive_sharing_mode) {
   unlink("test_file");
 
   ASSERT(UV_FS_O_EXLOCK > 0);
-
+  
   r = uv_fs_open(NULL,
                  &open_req1,
                  "test_file",

test/test-list.h

@@ -213,6 +213,7 @@ TEST_DECLARE   (active)
 TEST_DECLARE   (embed)
 TEST_DECLARE   (async)
 TEST_DECLARE   (async_null_cb)
+TEST_DECLARE   (eintr_handling)
 TEST_DECLARE   (get_currentexe)
 TEST_DECLARE   (process_title)
 TEST_DECLARE   (process_title_threadsafe)
@@ -324,8 +325,6 @@ TEST_DECLARE   (fs_read_write_null_arguments)
 TEST_DECLARE   (get_osfhandle_valid_handle)
 TEST_DECLARE   (fs_write_alotof_bufs)
 TEST_DECLARE   (fs_write_alotof_bufs_with_offset)
-TEST_DECLARE   (fs_partial_read)
-TEST_DECLARE   (fs_partial_write)
 TEST_DECLARE   (fs_file_pos_after_op_with_offset)
 TEST_DECLARE   (fs_null_req)
 #ifdef _WIN32
@@ -675,6 +674,7 @@ TASK_LIST_START
 
   TEST_ENTRY  (async)
   TEST_ENTRY  (async_null_cb)
+  TEST_ENTRY  (eintr_handling)
 
   TEST_ENTRY  (get_currentexe)
 
@@ -848,8 +848,6 @@ TASK_LIST_START
   TEST_ENTRY  (fs_write_multiple_bufs)
   TEST_ENTRY  (fs_write_alotof_bufs)
   TEST_ENTRY  (fs_write_alotof_bufs_with_offset)
-  TEST_ENTRY  (fs_partial_read)
-  TEST_ENTRY  (fs_partial_write)
   TEST_ENTRY  (fs_read_write_null_arguments)
   TEST_ENTRY  (fs_file_pos_after_op_with_offset)
   TEST_ENTRY  (fs_null_req)

uv.gyp

@@ -371,6 +371,7 @@
         'test/test-cwd-and-chdir.c',
         'test/test-default-loop-close.c',
         'test/test-delayed-accept.c',
+        'test/test-eintr-handling.c',
         'test/test-error.c',
         'test/test-embed.c',
         'test/test-emfile.c',