[PATCH v2 0/1] MR881: kernelbase: Fix reading stale information from IOSB.

[Jinoh Kang (＠iamahuman)]

Ensure IOSB Information (InternalHigh) is loaded after Status (Internal)
to avoid reading stale value from the Information (InternalHigh) field.

--
  v2: kernelbase: Fix reading stale information from IOSB.

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

[PATCH v2 1/1] kernelbase: Fix reading stale information from IOSB.

[Jinoh Kang]

From: Jinoh Kang

Ensure IOSB Information (InternalHigh) is loaded after Status (Internal)
to avoid reading stale value from the Information (InternalHigh) field.
---
 dlls/kernelbase/file.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c
index 5ba7e0be419..11dbcdeac4a 100644
--- a/dlls/kernelbase/file.c
+++ b/dlls/kernelbase/file.c
@@ -3133,7 +3133,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov

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

-    status = overlapped->Internal;
+    status = ReadAcquire( (volatile LONG *)&overlapped->Internal );
     if (status == STATUS_PENDING)
     {
         if (!timeout)
@@ -3150,7 +3150,13 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
             return FALSE;
         }

-        status = overlapped->Internal;
+        /* Normally this does not require an atomic load, since the wait above
+         * acts as a barrier as long as the caller supplied the correct handle
+         * and nothing else interfered with signalling of I/O completion.
+         * However, we do it anyway for consistency and extra safety in case
+         * Windows takes the same precaution for misbehaving applications.
+         */
+        status = ReadAcquire( (volatile LONG *)&overlapped->Internal );
         if (status == STATUS_PENDING) status = STATUS_SUCCESS;
     }

[PATCH v2 1/1] kernelbase: Fix reading stale information from IOSB.

[Zebediah Figura]

On 9/18/22 10:11, Jinoh Kang wrote:
> From: Jinoh Kang
>
> Ensure IOSB Information (InternalHigh) is loaded after Status (Internal)
> to avoid reading stale value from the Information (InternalHigh) field.
>
> dlls/kernelbase/file.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

I don't think this should be a separate patch set as merge request 155, 
and arguably not a separate commit from it either.
> diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c
> index 5ba7e0be419..11dbcdeac4a 100644
> --- a/dlls/kernelbase/file.c
> +++ b/dlls/kernelbase/file.c
> @@ -3133,7 +3133,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
>   TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );
>
> - status = overlapped->Internal;
> - status = ReadAcquire( (volatile LONG *)&overlapped->Internal );
>  if (status == STATUS_PENDING)
>  {
>      if (!timeout)

In general when using barriers I think it's good practice to comment (on 
both sides) where the other half of the barrier is.
> @@ -3150,7 +3150,13 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
>               return FALSE;
>           }
> -
>    status = overlapped->Internal;
> -
>    /* Normally this does not require an atomic load, since the wait above
> -
>     * acts as a barrier as long as the caller supplied the correct handle
> -
>     * and nothing else interfered with signalling of I/O completion.
> -
>     * However, we do it anyway for consistency and extra safety in case
> -
>     * Windows takes the same precaution for misbehaving applications.
> -
>     */
> -
>    status = ReadAcquire( (volatile LONG *)&overlapped->Internal );
>     if (status == STATUS_PENDING) status = STATUS_SUCCESS;
> }

I don't see how this can even make a difference?

[Jinoh Kang (＠iamahuman)]

> In general when using barriers I think it's good practice to comment (on both sides) where the other half of the barrier is.

ACK.
> I don't see how this can even make a difference?

Right, misbehaving apps misbehave.  My intention was to eliminate the possibility of a specific execution order that possibly *cannot* be observed at all on Windows, so that Wine's failure mode matches that of Windows.  I gave up anyway.

[PATCH v2 0/1] MR881: kernelbase: Fix reading stale information from IOSB. - closed

[Jinoh Kang (＠iamahuman)]

This merge request was closed by Jinoh Kang.