diff options
author | Evgeny Kotkov <kotkov@apache.org> | 2023-02-07 10:48:15 +0000 |
---|---|---|
committer | Evgeny Kotkov <kotkov@apache.org> | 2023-02-07 10:48:15 +0000 |
commit | a8a3f064f397a478a78dd51d614357e586b432b2 (patch) | |
tree | 79589288dc119e68f09ebef618062889761ca515 | |
parent | 1e83a9c3d4f05a537a4ed44aed6e9ce56405d98e (diff) | |
download | apr-a8a3f064f397a478a78dd51d614357e586b432b2.tar.gz |
On 1.7.x branch: Merge r1806604, r1806608 from trunk:
Fix a deadlock when writing to locked files opened with APR_FOPEN_APPEND
on Windows (PR 50058).
git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.7.x@1907497 13f79535-47bb-0310-9956-ffa450edef68
-rw-r--r-- | CHANGES | 5 | ||||
-rw-r--r-- | file_io/win32/readwrite.c | 43 | ||||
-rw-r--r-- | test/testfile.c | 167 |
3 files changed, 209 insertions, 6 deletions
@@ -1,6 +1,5 @@ -*- coding: utf-8 -*- Changes for APR 1.7.3 - *) apr_socket_sendfile: Use WSAIoctl() to get TransmitFile function pointer on Windows. [Ivan Zhakov] @@ -10,6 +9,10 @@ Changes for APR 1.7.3 *) apr_file_gets: Optimize for buffered files on Windows. [Evgeny Kotkov <evgeny.kotkov visualsvn.com>] + *) Fix a deadlock when writing to locked files opened with APR_FOPEN_APPEND + on Windows. PR 50058. + [Evgeny Kotkov <evgeny.kotkov visualsvn.com>] + Changes for APR 1.7.2 *) Correct a packaging issue in 1.7.1. The contents of the release were diff --git a/file_io/win32/readwrite.c b/file_io/win32/readwrite.c index f8c03ee42..7ff5325b0 100644 --- a/file_io/win32/readwrite.c +++ b/file_io/win32/readwrite.c @@ -303,7 +303,44 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf, a } return rv; } else { - if (!thefile->pipe) { + if (thefile->pipe) { + rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote, + thefile->pOverlapped); + } + else if (thefile->append && !thefile->pOverlapped) { + OVERLAPPED ov = {0}; + + /* If the file is opened for synchronous I/O, take advantage of the + * documented way to atomically append data by calling WriteFile() + * with both the OVERLAPPED.Offset and OffsetHigh members set to + * 0xFFFFFFFF. This avoids calling LockFile() that is otherwise + * required to avoid a race condition between seeking to the end + * and writing data. Not locking the file improves robustness of + * such appends and avoids a deadlock when appending to an already + * locked file, as described in PR50058. + * + * We use this approach only for files opened for synchronous I/O + * because in this case the I/O Manager maintains the current file + * position. Otherwise, the file offset returned or changed by + * the SetFilePointer() API is not guaranteed to be valid and that + * could, for instance, break apr_file_seek() calls after appending + * data. Sadly, if a file is opened for asynchronous I/O, this + * call doesn't update the OVERLAPPED.Offset member to reflect the + * actual offset used when appending the data (which we could then + * use to make seeking and other operations involving filePtr work). + * Therefore, when appending to files opened for asynchronous I/O, + * we still use the LockFile + SetFilePointer + WriteFile approach. + * + * References: + * https://bz.apache.org/bugzilla/show_bug.cgi?id=50058 + * https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747 + * https://msdn.microsoft.com/en-us/library/windows/hardware/ff567121 + */ + ov.Offset = MAXDWORD; + ov.OffsetHigh = MAXDWORD; + rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote, &ov); + } + else { apr_off_t offset = 0; apr_status_t rc; if (thefile->append) { @@ -335,10 +372,6 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf, a apr_thread_mutex_unlock(thefile->mutex); } } - else { - rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote, - thefile->pOverlapped); - } if (rv) { *nbytes = bwrote; rv = APR_SUCCESS; diff --git a/test/testfile.c b/test/testfile.c index 01f886d54..685803a17 100644 --- a/test/testfile.c +++ b/test/testfile.c @@ -21,6 +21,8 @@ #include "apr_general.h" #include "apr_poll.h" #include "apr_lib.h" +#include "apr_strings.h" +#include "apr_thread_proc.h" #include "testutil.h" #define DIRNAME "data" @@ -1472,6 +1474,169 @@ static void test_datasync_on_stream(abts_case *tc, void *data) } } +typedef struct thread_file_append_ctx_t { + apr_pool_t *pool; + const char *fname; + apr_size_t chunksize; + char val; + int num_writes; + char *errmsg; +} thread_file_append_ctx_t; + +static void * APR_THREAD_FUNC thread_file_append_func(apr_thread_t *thd, void *data) +{ + thread_file_append_ctx_t *ctx = data; + apr_status_t rv; + apr_file_t *f; + int i; + char *writebuf; + char *readbuf; + + rv = apr_file_open(&f, ctx->fname, + APR_FOPEN_READ | APR_FOPEN_WRITE | APR_FOPEN_APPEND, + APR_FPROT_OS_DEFAULT, ctx->pool); + if (rv) { + apr_thread_exit(thd, rv); + return NULL; + } + + writebuf = apr_palloc(ctx->pool, ctx->chunksize); + memset(writebuf, ctx->val, ctx->chunksize); + readbuf = apr_palloc(ctx->pool, ctx->chunksize); + + for (i = 0; i < ctx->num_writes; i++) { + apr_size_t bytes_written; + apr_size_t bytes_read; + apr_off_t offset; + + rv = apr_file_write_full(f, writebuf, ctx->chunksize, &bytes_written); + if (rv) { + apr_thread_exit(thd, rv); + return NULL; + } + /* After writing the data, seek back from the current offset and + * verify what we just wrote. */ + offset = -((apr_off_t)ctx->chunksize); + rv = apr_file_seek(f, APR_CUR, &offset); + if (rv) { + apr_thread_exit(thd, rv); + return NULL; + } + rv = apr_file_read_full(f, readbuf, ctx->chunksize, &bytes_read); + if (rv) { + apr_thread_exit(thd, rv); + return NULL; + } + if (memcmp(readbuf, writebuf, ctx->chunksize) != 0) { + ctx->errmsg = apr_psprintf( + ctx->pool, + "Unexpected data at file offset %" APR_OFF_T_FMT, + offset); + apr_thread_exit(thd, APR_SUCCESS); + return NULL; + } + } + + apr_file_close(f); + apr_thread_exit(thd, APR_SUCCESS); + + return NULL; +} + +static void test_atomic_append(abts_case *tc, void *data) +{ + apr_status_t rv; + apr_status_t thread_rv; + apr_file_t *f; + const char *fname = "data/testatomic_append.dat"; + unsigned int seed; + thread_file_append_ctx_t ctx1 = {0}; + thread_file_append_ctx_t ctx2 = {0}; + apr_thread_t *t1; + apr_thread_t *t2; + + apr_file_remove(fname, p); + + rv = apr_file_open(&f, fname, APR_FOPEN_WRITE | APR_FOPEN_CREATE, + APR_FPROT_OS_DEFAULT, p); + APR_ASSERT_SUCCESS(tc, "create file", rv); + apr_file_close(f); + + seed = (unsigned int)apr_time_now(); + abts_log_message("Random seed for test_atomic_append() is %u", seed); + srand(seed); + + /* Create two threads appending data to the same file. */ + apr_pool_create(&ctx1.pool, p); + ctx1.fname = fname; + ctx1.chunksize = 1 + rand() % 8192; + ctx1.val = 'A'; + ctx1.num_writes = 1000; + rv = apr_thread_create(&t1, NULL, thread_file_append_func, &ctx1, p); + APR_ASSERT_SUCCESS(tc, "create thread", rv); + + apr_pool_create(&ctx2.pool, p); + ctx2.fname = fname; + ctx2.chunksize = 1 + rand() % 8192; + ctx2.val = 'B'; + ctx2.num_writes = 1000; + rv = apr_thread_create(&t2, NULL, thread_file_append_func, &ctx2, p); + APR_ASSERT_SUCCESS(tc, "create thread", rv); + + rv = apr_thread_join(&thread_rv, t1); + APR_ASSERT_SUCCESS(tc, "join thread", rv); + APR_ASSERT_SUCCESS(tc, "no thread errors", thread_rv); + if (ctx1.errmsg) { + ABTS_FAIL(tc, ctx1.errmsg); + } + rv = apr_thread_join(&thread_rv, t2); + APR_ASSERT_SUCCESS(tc, "join thread", rv); + APR_ASSERT_SUCCESS(tc, "no thread errors", thread_rv); + if (ctx2.errmsg) { + ABTS_FAIL(tc, ctx2.errmsg); + } + + apr_file_remove(fname, p); +} + +static void test_append_locked(abts_case *tc, void *data) +{ + apr_status_t rv; + apr_file_t *f; + const char *fname = "data/testappend_locked.dat"; + apr_size_t bytes_written; + apr_size_t bytes_read; + char buf[64] = {0}; + + apr_file_remove(fname, p); + + rv = apr_file_open(&f, fname, + APR_FOPEN_WRITE | APR_FOPEN_CREATE | APR_FOPEN_APPEND, + APR_FPROT_OS_DEFAULT, p); + APR_ASSERT_SUCCESS(tc, "create file", rv); + + rv = apr_file_lock(f, APR_FLOCK_EXCLUSIVE); + APR_ASSERT_SUCCESS(tc, "lock file", rv); + + /* PR50058: Appending to a locked file should not deadlock. */ + rv = apr_file_write_full(f, "abc", 3, &bytes_written); + APR_ASSERT_SUCCESS(tc, "write to file", rv); + + apr_file_unlock(f); + apr_file_close(f); + + rv = apr_file_open(&f, fname, APR_FOPEN_READ, APR_FPROT_OS_DEFAULT, p); + APR_ASSERT_SUCCESS(tc, "open file", rv); + + rv = apr_file_read_full(f, buf, sizeof(buf), &bytes_read); + ABTS_INT_EQUAL(tc, APR_EOF, rv); + ABTS_INT_EQUAL(tc, 3, (int)bytes_read); + ABTS_STR_EQUAL(tc, "abc", buf); + + apr_file_close(f); + apr_file_remove(fname, p); +} + abts_suite *testfile(abts_suite *suite) { suite = ADD_SUITE(suite) @@ -1523,6 +1688,8 @@ abts_suite *testfile(abts_suite *suite) abts_run_test(suite, test_xthread, NULL); abts_run_test(suite, test_datasync_on_file, NULL); abts_run_test(suite, test_datasync_on_stream, NULL); + abts_run_test(suite, test_atomic_append, NULL); + abts_run_test(suite, test_append_locked, NULL); return suite; } |