[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 <jinoh.kang.kr(a)gmail.com>

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;
     }
 
-- 
GitLab

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

[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 <jinoh.kang.kr(a)gmail.com>
>
> 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.

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

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

[Jinoh Kang (＠iamahuman)]

This merge request was closed by Jinoh Kang.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/881