From 14bfc27e641aff178c431083c0c0eada4d6f02dd Mon Sep 17 00:00:00 2001 From: Ben Wijen Date: Fri, 22 Jan 2016 19:35:46 +0100 Subject: [PATCH] unix,fs: fix for potential partial reads/writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added a check in uv__fs_buf_iter to detect partial reads and writes. Partial reads and writes are looped until all data has been processed. PR-URL: https://github.com/libuv/libuv/pull/640 Reviewed-By: Santiago Gimeno Reviewed-By: Saúl Ibarra Corretgé --- 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, 150 insertions(+), 141 deletions(-) delete mode 100644 test/test-eintr-handling.c diff --git a/Makefile.am b/Makefile.am index ae9d96bc..c49802f7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -165,7 +165,6 @@ 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 e0969a4c..79864638 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -334,25 +334,7 @@ static ssize_t uv__fs_read(uv_fs_t* req) { if (no_preadv) retry: # endif { - 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; + result = pread(req->file, req->bufs[0].base, req->bufs[0].len, req->off); } # if defined(__linux__) else { @@ -740,25 +722,7 @@ static ssize_t uv__fs_write(uv_fs_t* req) { if (no_pwritev) retry: # endif { - 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; + r = pwrite(req->file, req->bufs[0].base, req->bufs[0].len, req->off); } # if defined(__linux__) else { @@ -1008,6 +972,19 @@ 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) { @@ -1027,7 +1004,10 @@ static ssize_t uv__fs_buf_iter(uv_fs_t* req, uv__fs_buf_iter_processor process) if (req->nbufs > iovmax) req->nbufs = iovmax; - result = process(req); + do + result = process(req); + while (result < 0 && errno == EINTR); + if (result <= 0) { if (total == 0) total = result; @@ -1037,14 +1017,12 @@ 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 deleted file mode 100644 index 1aaf623b..00000000 --- a/test/test-eintr-handling.c +++ /dev/null @@ -1,94 +0,0 @@ -/* 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 cae02dd1..fc568dc2 100644 --- a/test/test-fs.c +++ b/test/test-fs.c @@ -2865,6 +2865,131 @@ 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; @@ -3073,7 +3198,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 a008b039..ea3692c7 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -214,7 +214,6 @@ 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) @@ -325,6 +324,8 @@ 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,7 +676,6 @@ TASK_LIST_START TEST_ENTRY (async) TEST_ENTRY (async_null_cb) - TEST_ENTRY (eintr_handling) TEST_ENTRY (get_currentexe) @@ -848,6 +848,8 @@ 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 46606c5b..19008dfa 100644 --- a/uv.gyp +++ b/uv.gyp @@ -371,7 +371,6 @@ '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',