[PATCH v9 0/1] 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

--
  v9: ntdll: Fix reading stale Information from IOSB.

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

[PATCH v9 1/1] ntdll: Fix reading stale Information from IOSB.

[Jinoh Kang]

From: Jinoh Kang

Ensure proper atomic update, so that the application do not read stale
information from IO_STATUS_BLOCK.

Also, ensure IOSB Status loads are not reordered with Information loads
from the consumer side as well, specifically in the following functions:

- GetOverlappedResultEx
- WSAGetOverlappedResult
---
 dlls/kernelbase/file.c         |  3 ++-
 dlls/ntdll/unix/unix_private.h | 42 +++++++++++++++++++++++++++++++---
 dlls/ws2_32/socket.c           |  3 ++-
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c
index 3e1fd5aec4e..2d46926324e 100644
--- a/dlls/kernelbase/file.c
+++ b/dlls/kernelbase/file.c
@@ -3133,7 +3133,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov

TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );

-    status = overlapped->Internal;
+    /* Paired with write-release of IOSB Status in ntdll's set_async_iosb. */
+    status = ReadAcquire( &overlapped->Internal );
     if (status == STATUS_PENDING)
     {
         if (!timeout)
diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h
index 73b9ed76de0..3d032fba59b 100644
--- a/dlls/ntdll/unix/unix_private.h
+++ b/dlls/ntdll/unix/unix_private.h
@@ -361,6 +361,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())
@@ -370,19 +372,53 @@ 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() et al. 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.
+     *
+     * CPU 0 (set_async_iosb)       CPU 1 (GetOverlappedResult)
+     * WRITE(Information)
+     * <release barrier>   <--.
+     * WRITE(Status)          |
+     *                        |
+     *                        |     READ(Status)
+     *         (paired with)  `-->  <acquire barrier>
+     *                              READ(Information)
+     *
+     * 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.
+     *
+     * CPU 0 (set_async_iosb)       CPU 1 (GetOverlappedResult)
+     * WRITE(Status)
+     *                              READ(Status)
+     *                              READ(Information) <-- (reads stale value!)
+     * WRITE(Information) <-- (reordered)
+     *
+     * The race condition can be especially problematic in applications that
+     * use the HasOverlappedIoCompleted() macro in a tight loop to poll for
+     * completion of overlapped I/O.
+     *
+     * Paired with GetOverlappedResultEx and WSAGetOverlappedResult.
+     */
+    WriteRelease( status_ptr, status );
 }

static inline client_ptr_t iosb_client_ptr( IO_STATUS_BLOCK *io )
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 2f115f02e48..592fc841555 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -3702,7 +3702,8 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped,
         return FALSE;
     }

-    status = lpOverlapped->Internal;
+    /* Paired with write-release of IOSB Status in ntdll's set_async_iosb. */
+    status = ReadAcquire( &lpOverlapped->Internal );
     if (status == STATUS_PENDING)
     {
         if (!fWait)

[Jinoh Kang (＠iamahuman)]

I added memory access sequence diagrams in comments.