From a37559fdde9d08aad081a28f3118273f61f5a360 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 19 Jan 2018 01:24:43 +0100 Subject: [PATCH 2/3] 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 14bfc27e641aff178c431083c0c0eada4d6f02dd. Fixes: https://github.com/libuv/libuv/issues/1716 Fixes: https://github.com/libuv/libuv/issues/1720 Fixes: https://github.com/nodejs/node/issues/18225 --- Makefile.am | 1 + src/unix/fs.c | 62 +++++++++++++++------- test/test-eintr-handling.c | 94 +++++++++++++++++++++++++++++++++ test/test-fs.c | 127 +-------------------------------------------- test/test-list.h | 6 +-- uv.gyp | 1 + 6 files changed, 141 insertions(+), 150 deletions(-) create mode 100644 test/test-eintr-handling.c diff --git a/Makefile.am b/Makefile.am index c49802f7..ae9d96bc 100644 --- a/Makefile.am +++ b/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 \ diff --git a/src/unix/fs.c b/src/unix/fs.c index 79864638..e0969a4c 100644 --- a/src/unix/fs.c +++ b/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); - while (result < 0 && errno == EINTR); - + result = process(req); 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); diff --git a/test/test-eintr-handling.c b/test/test-eintr-handling.c new file mode 100644 index 00000000..1aaf623b --- /dev/null +++ b/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 +#include + +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 */ diff --git a/test/test-fs.c b/test/test-fs.c index d43d6e61..241416bc 100644 --- a/test/test-fs.c +++ b/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", diff --git a/test/test-list.h b/test/test-list.h index 8e4f2025..5a50ec67 100644 --- a/test/test-list.h +++ b/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) diff --git a/uv.gyp b/uv.gyp index 19008dfa..46606c5b 100644 --- a/uv.gyp +++ b/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', -- 2.15.1