[PATCH v3 0/2] MR155: ntdll: Fix reading stale Information from IOSB.

[Jinoh Kang (＠iamahuman)]

Enforce proper atomic update so that other threads do not read stale
data from IO_STATUS_BLOCK.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>

--
  v3: ntdll: Fix reading stale Information from IOSB.
      include: Define atomic read/write helpers for 32-bit integers.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/155

[PATCH v3 1/2] include: Define atomic read/write helpers for 32-bit integers.

[Jinoh Kang]

From: Jinoh Kang <jinoh.kang.kr(a)gmail.com>

Based on the corresponding functions from Windows SDK header files.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---
 include/winnt.h | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/include/winnt.h b/include/winnt.h
index 87c4b4da92d..a98201dc3b0 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -6475,6 +6475,70 @@ static FORCEINLINE void MemoryBarrier(void)
 
 #endif /* __i386__ */
 
+#if defined(__i386__) || defined(__x86_64__)
+
+static FORCEINLINE LONG ReadAcquire( LONG const volatile *src )
+{
+    LONG value = *src;
+    return value;
+}
+
+static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
+{
+    LONG value = *src;
+    return value;
+}
+
+static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value )
+{
+    *dest = value;
+}
+
+static FORCEINLINE void WriteNoFence( LONG volatile *dest, LONG value )
+{
+    *dest = value;
+}
+
+#else
+
+/* __iso_volatile_* intrinsics do not need explicit declarations. */
+#pragma intrinsic(__iso_volatile_load32)
+#pragma intrinsic(__iso_volatile_store32)
+
+#if defined(__arm__)
+#define WINE_MEMORY_BARRIER_ACQUIRE() __dmb(_ARM_BARRIER_ISH)
+#define WINE_MEMORY_BARRIER_RELEASE() __dmb(_ARM_BARRIER_ISH)
+#elif defined(__aarch64__)
+#define WINE_MEMORY_BARRIER_ACQUIRE() __dmb(_ARM64_BARRIER_ISH)
+#define WINE_MEMORY_BARRIER_RELEASE() __dmb(_ARM64_BARRIER_ISH)
+#endif  /* defined(__arm__) */
+
+static FORCEINLINE LONG ReadAcquire( LONG const volatile *src )
+{
+    LONG value = __iso_volatile_load32( (int const volatile *)src );
+    WINE_MEMORY_BARRIER_ACQUIRE();
+    return value;
+}
+
+static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
+{
+    LONG value = __iso_volatile_load32( (int const volatile *)src );
+    return value;
+}
+
+static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value )
+{
+    WINE_MEMORY_BARRIER_RELEASE();
+    __iso_volatile_store32( (int volatile *)dest, value );
+}
+
+static FORCEINLINE void WriteNoFence( LONG volatile *dest, LONG value )
+{
+    __iso_volatile_store32( (int volatile *)dest, value );
+}
+
+#endif  /* defined(__i386__) || defined(__x86_64__) */
+
 #elif defined(__GNUC__)
 
 static FORCEINLINE BOOLEAN WINAPI BitScanForward(DWORD *index, DWORD mask)
@@ -6588,6 +6652,43 @@ static FORCEINLINE void MemoryBarrier(void)
     __sync_synchronize();
 }
 
+#if defined(__x86_64__) || defined(__i386__)
+/* On x86, Support old GCC with either no or buggy (GCC BZ#81316) __atomic_* support */
+#define WINE_ATOMIC_LOAD_ACQUIRE(ptr, ret) do { *(ret) = *(ptr); __asm__ __volatile__( "" ::: "memory" ); } while (0)
+#define WINE_ATOMIC_LOAD_RELAXED(ptr, ret) do { *(ret) = *(ptr); } while (0)
+#define WINE_ATOMIC_STORE_RELEASE(ptr, val) do { __asm__ __volatile__( "" ::: "memory" ); *(ptr) = *(val); } while (0)
+#define WINE_ATOMIC_STORE_RELAXED(ptr, val) do { *(ptr) = *(val); } while (0)
+#else
+#define WINE_ATOMIC_LOAD_ACQUIRE(ptr, ret) __atomic_load(ptr, ret, __ATOMIC_ACQUIRE)
+#define WINE_ATOMIC_LOAD_RELAXED(ptr, ret) __atomic_load(ptr, ret, __ATOMIC_RELAXED)
+#define WINE_ATOMIC_STORE_RELEASE(ptr, val) __atomic_store(ptr, val, __ATOMIC_RELEASE)
+#define WINE_ATOMIC_STORE_RELAXED(ptr, val) __atomic_store(ptr, val, __ATOMIC_RELAXED)
+#endif  /* defined(__x86_64__) || defined(__i386__) */
+
+static FORCEINLINE LONG ReadAcquire( LONG const volatile *src )
+{
+    LONG value;
+    WINE_ATOMIC_LOAD_ACQUIRE( src, &value );
+    return value;
+}
+
+static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
+{
+    LONG value;
+    WINE_ATOMIC_LOAD_RELAXED( src, &value );
+    return value;
+}
+
+static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value )
+{
+    WINE_ATOMIC_STORE_RELEASE( dest, &value );
+}
+
+static FORCEINLINE void WriteNoFence( LONG volatile *dest, LONG value )
+{
+    WINE_ATOMIC_STORE_RELAXED( dest, &value );
+}
+
 static FORCEINLINE DECLSPEC_NORETURN void __fastfail(unsigned int code)
 {
 #if defined(__x86_64__) || defined(__i386__)
@@ -6603,6 +6704,26 @@ static FORCEINLINE DECLSPEC_NORETURN void __fastfail(unsigned int code)
 
 #endif  /* __GNUC__ */
 
+static FORCEINLINE ULONG ReadULongAcquire( ULONG const volatile *src )
+{
+    return (ULONG)ReadAcquire( (LONG const volatile *)src );
+}
+
+static FORCEINLINE ULONG ReadULongNoFence( ULONG const volatile *src )
+{
+    return (ULONG)ReadNoFence( (LONG const volatile *)src );
+}
+
+static FORCEINLINE void WriteULongRelease( ULONG volatile *dest, ULONG value )
+{
+    WriteRelease( (LONG volatile *)dest, value );
+}
+
+static FORCEINLINE void WriteULongNoFence( ULONG volatile *dest, ULONG value )
+{
+    WriteNoFence( (LONG volatile *)dest, value );
+}
+
 #ifdef _WIN64
 
 #define InterlockedCompareExchange128 _InterlockedCompareExchange128
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/155

[PATCH v3 1/2] include: Define atomic read/write helpers for 32-bit integers.

[Jinoh Kang]

Forgot that this PR wasn't a draft.  Sorry for the noise...

[PATCH v3 2/2] ntdll: Fix reading stale Information from IOSB.

[Jinoh Kang]

From: Jinoh Kang <jinoh.kang.kr(a)gmail.com>

Enforce proper atomic update so that other threads do not read stale
data from IO_STATUS_BLOCK.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---
 dlls/ntdll/unix/unix_private.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h
index 795fc148479..0c2a5e065ae 100644
--- a/dlls/ntdll/unix/unix_private.h
+++ b/dlls/ntdll/unix/unix_private.h
@@ -357,6 +357,8 @@ static inline BOOL in_wow64_call(void)
 
 static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR info )
 {
+    NTSTATUS *status_ptr;
+
     if (!iosb) return;
 
     if (in_wow64_call())
@@ -366,19 +368,36 @@ static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR
             NTSTATUS Status;
             ULONG    Information;
         } *io = wine_server_get_ptr( iosb );
-        io->Status = status;
+        status_ptr = &io->Status;
         io->Information = info;
     }
     else
     {
         IO_STATUS_BLOCK *io = wine_server_get_ptr( iosb );
 #ifdef NONAMELESSUNION
-        io->u.Status = status;
+        status_ptr = &io->u.Status;
 #else
-        io->Status = status;
+        status_ptr = &io->Status;
 #endif
         io->Information = info;
     }
+
+    /* GetOverlappedResultEx() skips waiting for the event/file handle if the
+     * Status field indicates completion, and returns the result from the
+     * Information field.
+     *
+     * Hence, we have to ensure that writes to the Information field are seen
+     * from the other threads *before* writes to the Status field.
+     *
+     * Otherwise, a race condition results: if writing the Status field has
+     * been completed but the subsequent update to the Information field has
+     * not, the application may end up reading stale value from it.
+     *
+     * The race condition can be especially problematic with applications that
+     * use the HasOverlappedIoCompleted() macro in a tight loop to busy-wait
+     * for completion of overlapped I/O.
+     */
+    WriteRelease( (volatile LONG *)status_ptr, status );
 }
 
 static inline client_ptr_t iosb_client_ptr( IO_STATUS_BLOCK *io )
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/155