From: Jinoh Kang jinoh.kang.kr@gmail.com
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 | 44 +++++++++++++++++++++++++++++++--- dlls/ws2_32/socket.c | 3 ++- 3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index ee6b499304b..81917b6936f 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -3137,7 +3137,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );
- status = io->iosb.u.Status; + /* Paired with write-release of IOSB Status in ntdll's set_async_iosb. */ + status = ReadAcquire( &io->iosb.u.Status ); if (status == STATUS_PENDING) { if (!timeout) diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 73b9ed76de0..1641337dd19 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,55 @@ 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) <-- (stale read!) + * WRITE(Information) <-- (reordered from above) + * + * 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 be491a32b97..568eedf1a40 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -3706,7 +3706,8 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped, return FALSE; }
- status = io->iosb.u.Status; + /* Paired with write-release of IOSB Status in ntdll's set_async_iosb. */ + status = ReadAcquire( &io->iosb.u.Status ); if (status == STATUS_PENDING) { if (!fWait)