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

3 files changed, 209 insertions, 6 deletions

diff --git a/CHANGES b/CHANGES
index f55b88017..9d52bb81a 100644
--- a/CHANGES
+++ b/CHANGES
@@ -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;
 }