You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by iv...@apache.org on 2022/06/30 16:52:00 UTC
svn commit: r1902376 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Author: ivan
Date: Thu Jun 30 16:52:00 2022
New Revision: 1902376
URL: http://svn.apache.org/viewvc?rev=1902376&view=rev
Log:
On 1.8.x branch: Merge r1806608 from trunk:
Win32: Fix a deadlock when appending to locked files (PR50058).
Modified:
apr/apr/branches/1.8.x/ (props changed)
apr/apr/branches/1.8.x/CHANGES
apr/apr/branches/1.8.x/file_io/win32/readwrite.c
apr/apr/branches/1.8.x/test/testfile.c
Propchange: apr/apr/branches/1.8.x/
------------------------------------------------------------------------------
Merged /apr/apr/trunk:r1806608
Modified: apr/apr/branches/1.8.x/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.8.x/CHANGES?rev=1902376&r1=1902375&r2=1902376&view=diff
==============================================================================
--- apr/apr/branches/1.8.x/CHANGES [utf-8] (original)
+++ apr/apr/branches/1.8.x/CHANGES [utf-8] Thu Jun 30 16:52:00 2022
@@ -13,6 +13,10 @@ Changes for APR 1.8.0
*) Fix double free on exit when apr_app is used on Windows. [Ivan Zhakov]
+ *) Fix a deadlock when writing to locked files opened with APR_FOPEN_APPEND
+ on Windows. PR 50058.
+ [Evgeny Kotkov <evgeny.kotkov visualsvn.com>]
+
*) apr_file_gets: Optimize for buffered files on Windows.
[Evgeny Kotkov <evgeny.kotkov visualsvn.com>]
Modified: apr/apr/branches/1.8.x/file_io/win32/readwrite.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.8.x/file_io/win32/readwrite.c?rev=1902376&r1=1902375&r2=1902376&view=diff
==============================================================================
--- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
+++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30 16:52:00 2022
@@ -308,6 +308,39 @@ APR_DECLARE(apr_status_t) apr_file_write
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;
Modified: apr/apr/branches/1.8.x/test/testfile.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.8.x/test/testfile.c?rev=1902376&r1=1902375&r2=1902376&view=diff
==============================================================================
--- apr/apr/branches/1.8.x/test/testfile.c (original)
+++ apr/apr/branches/1.8.x/test/testfile.c Thu Jun 30 16:52:00 2022
@@ -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"
@@ -1540,6 +1542,175 @@ static void test_append(abts_case *tc, v
apr_file_close(f2);
}
+#if APR_HAS_THREADS
+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;
+}
+#endif /* APR_HAS_THREADS */
+
+static void test_atomic_append(abts_case *tc, void *data)
+{
+#if APR_HAS_THREADS
+ 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);
+#else
+ ABTS_SKIP(tc, data, "This test requires APR thread support.");
+#endif /* APR_HAS_THREADS */
+}
+
+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)
@@ -1592,6 +1763,8 @@ abts_suite *testfile(abts_suite *suite)
abts_run_test(suite, test_append, 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;
}