[PATCH v3 0/1] MR4310: ntdll: Tweak the binary representation of SRWLOCK.
There are applications that uses SRWLOCK in an invalid way and then checks its binary representation. ~~Specifically they releases an unlocked SRWLOCK then check its bit pattern is all-ones.~~ Tweak the representation a bit so they are happy. -- v3: ntdll: Tweak the binary representation of SRWLOCK. https://gitlab.winehq.org/wine/wine/-/merge_requests/4310
From: Yuxuan Shui <yshui(a)codeweavers.com> There are applications that uses SRWLOCK in an invalid way and then checks its binary representation. Tweak our representation a bit so they are happy. --- dlls/ntdll/sync.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index fa64917029a..5ae4fb4def9 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -473,8 +473,6 @@ DWORD WINAPI RtlRunOnceExecuteOnce( RTL_RUN_ONCE *once, PRTL_RUN_ONCE_INIT_FN fu struct srw_lock { - short exclusive_waiters; - /* Number of shared owners, or -1 if owned exclusive. * * Sadly Windows has no equivalent to FUTEX_WAIT_BITSET, so in order to wake @@ -483,10 +481,11 @@ struct srw_lock * RtlAcquireSRWLockShared() needs to know the values of "exclusive_waiters" * and "owners", but RtlAcquireSRWLockExclusive() only needs to know the * value of "owners", so the former can wait on the entire structure, and - * the latter waits only on the "owners" member. Note then that "owners" - * must not be the first element in the structure. + * the latter waits only on the "owners" member. */ short owners; + + short exclusive_waiters; }; C_ASSERT( sizeof(struct srw_lock) == 4 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4310
On Mon Nov 6 16:28:16 2023 +0000, Yuxuan Shui wrote:
OK, I looked around a bit. `WaitOnAddress` has a size parameter, isn't that enough to indicate waiting on `owners` only? We don't need the non-four-byte-aligned-ness for that. The problem is that RtlWakeByAddress doesn't specify a size. If 'owners' is the first field, we can't distinguish between waking exclusive and shared waiters anymore. That's correct (I think?) but it means more spurious wakeups.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51115
On Mon Nov 6 17:36:16 2023 +0000, Zebediah Figura wrote:
The problem is that RtlWakeByAddress doesn't specify a size. If 'owners' is the first field, we can't distinguish between waking exclusive and shared waiters anymore. That's correct (I think?) but it means more spurious wakeups. I see, I don't see a simple way around this. Would it be really bad if we have an internal `WakeByAddress` that takes a size?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51117
On Mon Nov 6 17:41:07 2023 +0000, Yuxuan Shui wrote:
I see, I don't see a simple way around this. Would it be really bad if we have an internal `WakeByAddress` that takes a size? All other solutions I can think of involve changing the design of SRWLOCK.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51118
On Mon Nov 6 17:42:10 2023 +0000, Yuxuan Shui wrote:
All other solutions I can think of involve changing the design of SRWLOCK. Or... We can use the lowest bit of `exclusive_waiters` to indicate if lock is exclusively held. This feels really unnatural because we would be storing redundant information. But if you are fine with it...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51122
I see, I don't see a simple way around this. Would it be really bad if we have an internal `WakeByAddress` that takes a size?
Well, we'd need to store the size in the futex wait queue, which is a bit strange. At that rate it may make more sense to have dedicated wait queues for SRW locks. That'd end up being something closer to 3504, but not necessarily with all of the complexity of (or even compatibility with) the Windows implementation. One other note is that regardless of what path we take, I think we should keep the test that replicates what the offending application is doing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51123
participants (3)
-
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Zebediah Figura (＠zfigura)