Merge r1868129, r1868502 from trunk:

apr_atomic_read64(): Fix non-atomic read on 32-bit Windows.

* atomic/win32/apr_atomic64.c
  (apr_atomic_read64): Use InterlockedCompareExchange64() instead of direct
   memory read.

* test/testatomic.c
  (test_atomics_threaded_setread64): New test.
  (test_func_set64): Helepr for test_atomics_threaded_setread64 test.

* CHANGES: Add changelog entry.


* atomic/win32/apr_atomic64.c
  (apr_atomic_read64): Use direct memory read when compiled for x86_x64, since
   64-bit reads are atomic in 64-bit Windows [1].

Suggested by: Yann Ylavic

[1] https://docs.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access


Submitted by: ivan
Reviewed by: ylavic


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.7.x@1896068 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 49 insertions, 0 deletions

diff --git a/CHANGES b/CHANGES
index 7b5f3860d..49ac16aa8 100644
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,8 @@ Changes for APR 1.7.1
      (This issue was addressed as CVE-2017-12613 in APR 1.6.3 and
      later 1.6.x releases, but was missing in 1.7.0.)  [Stefan Sperling]
 
+  *) apr_atomic_read64(): Fix non-atomic read on 32-bit Windows [Ivan Zhakov]
+
   *) configure: Add --disable-sctp argument to forcibly disable SCTP
      support, or --enable-sctp which fails if SCTP support is not
      detected.  [Lubos Uhliarik <luhliari redhat.com>, Joe Orton]
diff --git a/atomic/win32/apr_atomic64.c b/atomic/win32/apr_atomic64.c
index e2cd06d6c..372003aaa 100644
--- a/atomic/win32/apr_atomic64.c
+++ b/atomic/win32/apr_atomic64.c
@@ -51,7 +51,16 @@ APR_DECLARE(void) apr_atomic_set64(volatile apr_uint64_t *mem, apr_uint64_t val)
 
 APR_DECLARE(apr_uint64_t) apr_atomic_read64(volatile apr_uint64_t *mem)
 {
+#if defined(_M_X64)
+    /* https://docs.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access
+     * "Simple reads and writes to properly aligned 64-bit variables are atomic
+     * on 64-bit Windows."*/
     return *mem;
+#else
+    /* 64-bit read is not atomic on 32-bit platform: use InterlockedCompareExchange
+       to perform atomic read. */
+    return InterlockedCompareExchange64((volatile LONG64 *)mem, 0, 0);
+#endif
 }
 
 APR_DECLARE(apr_uint64_t) apr_atomic_cas64(volatile apr_uint64_t *mem, apr_uint64_t with,
diff --git a/test/testatomic.c b/test/testatomic.c
index c2f1e46da..b66129924 100644
--- a/test/testatomic.c
+++ b/test/testatomic.c
@@ -876,6 +876,43 @@ static void test_atomics_busyloop_threaded64(abts_case *tc, void *data)
     ABTS_ASSERT(tc, "Failed creating threads", rv == APR_SUCCESS);
 }
 
+void *APR_THREAD_FUNC test_func_set64(apr_thread_t *thd, void *data)
+{
+    int i;
+
+    for (i = 0; i < 1000 * 1000; i++) {
+        apr_atomic_set64(&atomic_ops64, APR_UINT64_C(0x1111222233334444));
+        apr_atomic_set64(&atomic_ops64, APR_UINT64_C(0x4444555566667777));
+    }
+
+    apr_thread_exit(thd, APR_SUCCESS);
+    return NULL;
+}
+
+static void test_atomics_threaded_setread64(abts_case *tc, void *data)
+{
+    apr_status_t retval;
+    apr_thread_t *thread;
+    int i;
+
+    apr_atomic_set64(&atomic_ops64, APR_UINT64_C(0x1111222233334444));
+
+    apr_thread_create(&thread, NULL, test_func_set64, NULL, p);
+
+    for (i = 0; i < 1000 * 1000 * 2; i++) {
+        apr_uint64_t val = apr_atomic_read64(&atomic_ops64);
+
+        if (val != APR_UINT64_C(0x1111222233334444) &&
+            val != APR_UINT64_C(0x4444555566667777))
+        {
+            ABTS_FAIL(tc, "Unexpected value");
+            break;
+        }
+    }
+
+    apr_thread_join(&retval, thread);
+}
+
 #endif /* !APR_HAS_THREADS */
 
 abts_suite *testatomic(abts_suite *suite)
@@ -916,6 +953,7 @@ abts_suite *testatomic(abts_suite *suite)
     abts_run_test(suite, test_atomics_threaded64, NULL);
     abts_run_test(suite, test_atomics_busyloop_threaded, NULL);
     abts_run_test(suite, test_atomics_busyloop_threaded64, NULL);
+    abts_run_test(suite, test_atomics_threaded_setread64, NULL);
 #endif
 
     return suite;