[PATCH 0/1] MR1002: win32u: Fix data race in NtUserGetProcessDpiAwarenessContext.
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/win32u/sysparams.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 494bb94c7f1..03662e5506e 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -5326,14 +5326,17 @@ BOOL WINAPI NtUserSetProcessDpiAwarenessContext( ULONG awareness, ULONG unknown */ ULONG WINAPI NtUserGetProcessDpiAwarenessContext( HANDLE process ) { + DPI_AWARENESS val; + if (process && process != GetCurrentProcess()) { WARN( "not supported on other process %p\n", process ); return NTUSER_DPI_UNAWARE; } - if (!dpi_awareness) return NTUSER_DPI_UNAWARE; - return dpi_awareness; + val = ReadAcquire( &dpi_awareness ); + if (!val) return NTUSER_DPI_UNAWARE; + return val; } BOOL message_beep( UINT i ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1002
On 10/3/22 07:45, Jinoh Kang wrote:
@@ -5326,14 +5326,17 @@ BOOL WINAPI NtUserSetProcessDpiAwarenessContext( ULONG awareness, ULONG unknown */ ULONG WINAPI NtUserGetProcessDpiAwarenessContext( HANDLE process ) { + DPI_AWARENESS val; + if (process && process != GetCurrentProcess()) { WARN( "not supported on other process %p\n", process ); return NTUSER_DPI_UNAWARE; }
- if (!dpi_awareness) return NTUSER_DPI_UNAWARE; - return dpi_awareness; + val = ReadAcquire( &dpi_awareness ); + if (!val) return NTUSER_DPI_UNAWARE; + return val;
Why does this need acquire semantics?
On Wed, Oct 12, 2022 at 3:08 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 10/3/22 07:45, Jinoh Kang wrote:
@@ -5326,14 +5326,17 @@ BOOL WINAPI NtUserSetProcessDpiAwarenessContext( ULONG awareness, ULONG unknown */ ULONG WINAPI NtUserGetProcessDpiAwarenessContext( HANDLE process ) { + DPI_AWARENESS val; + if (process && process != GetCurrentProcess()) { WARN( "not supported on other process %p\n", process ); return NTUSER_DPI_UNAWARE; }
- if (!dpi_awareness) return NTUSER_DPI_UNAWARE; - return dpi_awareness; + val = ReadAcquire( &dpi_awareness ); + if (!val) return NTUSER_DPI_UNAWARE; + return val;
Why does this need acquire semantics?
It was intended to ensure that loads by two successive NtUserGetProcessDpiAwarenessContext() calls don't get reordered with each other. If you think this won't be a problem (i.e. such reordering cannot actually happen in C++ memory model, or the user is responsible for serializing access), I'll downgrade it to ReadNoFence.
On 10/15/22 09:26, Jin-oh Kang wrote:
On Wed, Oct 12, 2022 at 3:08 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 10/3/22 07:45, Jinoh Kang wrote:
@@ -5326,14 +5326,17 @@ BOOL WINAPI NtUserSetProcessDpiAwarenessContext( ULONG awareness, ULONG unknown */ ULONG WINAPI NtUserGetProcessDpiAwarenessContext( HANDLE process ) { + DPI_AWARENESS val; + if (process && process != GetCurrentProcess()) { WARN( "not supported on other process %p\n", process ); return NTUSER_DPI_UNAWARE; }
- if (!dpi_awareness) return NTUSER_DPI_UNAWARE; - return dpi_awareness; + val = ReadAcquire( &dpi_awareness ); + if (!val) return NTUSER_DPI_UNAWARE; + return val;
Why does this need acquire semantics?
It was intended to ensure that loads by two successive NtUserGetProcessDpiAwarenessContext() calls don't get reordered with each other.
If you think this won't be a problem (i.e. such reordering cannot actually happen in C++ memory model, or the user is responsible for serializing access), I'll downgrade it to ReadNoFence.
According to my reading this is prohibited most particularly by § 5.1.2.4.22 of the C11 (draft) specification: "Furthermore, if a value computation A of an atomic object M happens before a value computation B of M, and the value computed by A corresponds to the value stored by side effect X, then the value computed by B shall either equal the value computed by A, or be the value stored by side effect Y, where Y follows X in the modification order of M." The following note also suggests that this reading is the correct one: "This effectively disallows compiler reordering of atomic operations to a single object, even if both operations are ‘‘relaxed’’ loads." The prototypical use of acquire/release is to protect a *different* variable, which may or may not be atomic. Note also that an acquire with no corresponding release is highly suspicious.
Can you explain the race here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1002#note_10234
On Tue Oct 11 11:27:57 2022 +0000, Huw Davies wrote:
Can you explain the race here? It reads a shared variable *twice* without atomic access. If the first read resulted in a nonzero value but the second read resulted in zero, the function will return zero as-is instead of NTUSER_DPI_UNAWARE. Unless I have mistaken, this can happen if the two reads are reordered, notwithstanding the fact that the variable cannot normally transition from a nonzero value back to zero.
The variable cannot be made to default to NTUSER_DPI_UNAWARE without an extra out-of-band flag variable, since `NtUserSetProcessDpiAwarenessContext` has to distinguish between unset state (represented as zero) and set state (represented as nonzero). The reason why the process-wide DPI awareness state cannot be changed once set is that doing so will result in inconsistent DPI-aware behaviour between different application components each of which calls `NtUserGetProcessDpiAwarenessContext` to fetch the DPI awareness status. Also, the load-acquire is paired with `InterlockedCompareExchange` in `NtUserZetProcessDpiAwarenessContext` and is also necessary to prevent reordering across successive `NtUserGetProcessDpiAwarenessContext` calls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1002#note_10280
On Tue Oct 11 11:27:57 2022 +0000, Jinoh Kang wrote:
It reads a shared variable *twice* without atomic access. If the first read resulted in a nonzero value but the second read resulted in zero, the function will return zero as-is instead of NTUSER_DPI_UNAWARE. Unless I have mistaken, this can happen if the two reads are reordered, notwithstanding the fact that the variable cannot normally transition from a nonzero value back to zero. The variable cannot be made to default to NTUSER_DPI_UNAWARE without an extra out-of-band flag variable, since `NtUserSetProcessDpiAwarenessContext` has to distinguish between unset state (represented as zero) and set state (represented as nonzero). The reason why the process-wide DPI awareness state cannot be changed once set is that doing so will result in inconsistent DPI-aware behaviour between different application components each of which calls `NtUserGetProcessDpiAwarenessContext` to fetch the DPI awareness status. Also, the load-acquire is paired with `InterlockedCompareExchange` in `NtUserZetProcessDpiAwarenessContext` and is also necessary to prevent reordering across successive `NtUserGetProcessDpiAwarenessContext` calls. Right, but can the two reads really be reordered here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1002#note_10300
On Tue Oct 11 15:57:05 2022 +0000, Huw Davies wrote:
Right, but can the two reads really be reordered here? To be clear, I don't object to this patch. It is sematically correct and it does have the advantage of changing `NtUserGetProcessDpiAwarenessContext()` so that one doesn't need to understand that `dpi_awareness` only transitions at most once. I'm mainly interested about whether it's strictly necessary.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1002#note_10306
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1002
participants (5)
-
Huw Davies (＠huw) -
Jin-oh Kang -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Zebediah Figura