[PATCH v14 0/2] 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. -- v14: ntdll: Tweak the binary representation of SRWLOCK. https://gitlab.winehq.org/wine/wine/-/merge_requests/4310
From: Yuxuan Shui <yshui(a)codeweavers.com> --- include/winnt.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/winnt.h b/include/winnt.h index 7bbf224e3c9..3f02cf93d6e 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -6756,6 +6756,7 @@ typedef enum _FIRMWARE_TYPE #define InterlockedDecrement64 _InterlockedDecrement64 #define InterlockedExchange _InterlockedExchange #define InterlockedExchangeAdd _InterlockedExchangeAdd +#define InterlockedExchangeAdd16 _InterlockedExchangeAdd16 #define InterlockedExchangeAdd64 _InterlockedExchangeAdd64 #define InterlockedExchangePointer _InterlockedExchangePointer #define InterlockedIncrement _InterlockedIncrement @@ -6776,6 +6777,7 @@ typedef enum _FIRMWARE_TYPE #pragma intrinsic(_InterlockedCompareExchangePointer) #pragma intrinsic(_InterlockedExchange) #pragma intrinsic(_InterlockedExchangeAdd) +#pragma intrinsic(_InterlockedExchangeAdd16) #pragma intrinsic(_InterlockedExchangePointer) #pragma intrinsic(_InterlockedIncrement) #pragma intrinsic(_InterlockedIncrement16) @@ -6795,6 +6797,7 @@ long _InterlockedDecrement(long volatile*); short _InterlockedDecrement16(short volatile*); long _InterlockedExchange(long volatile*,long); long _InterlockedExchangeAdd(long volatile*,long); +short _InterlockedExchangeAdd16(short volatile*,short); void * _InterlockedExchangePointer(void *volatile*,void*); long _InterlockedIncrement(long volatile*); short _InterlockedIncrement16(short volatile*); @@ -7027,6 +7030,11 @@ static FORCEINLINE LONG WINAPI InterlockedExchangeAdd( LONG volatile *dest, LONG return __sync_fetch_and_add( dest, incr ); } +static FORCEINLINE SHORT WINAPI InterlockedExchangeAdd16( SHORT volatile *dest, SHORT incr ) +{ + return __sync_fetch_and_add( dest, incr ); +} + static FORCEINLINE LONGLONG WINAPI InterlockedExchangeAdd64( LONGLONG volatile *dest, LONGLONG incr ) { return __sync_fetch_and_add( dest, incr ); -- GitLab 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/kernel32/tests/sync.c | 16 ++++++++++++++++ dlls/ntdll/sync.c | 24 ++++++++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c index 10765765bc5..0b800cb0163 100644 --- a/dlls/kernel32/tests/sync.c +++ b/dlls/kernel32/tests/sync.c @@ -2535,6 +2535,21 @@ static void test_srwlock_example(void) trace("number of total exclusive accesses is %ld\n", srwlock_protected_value); } +static void test_srwlock_quirk(void) +{ + union { SRWLOCK *s; LONG *l; } u = { &srwlock_example }; + + if (!pInitializeSRWLock) { + /* function is not yet in XP, only in newer Windows */ + win_skip("no srw lock support.\n"); + return; + } + + *u.l = 1; + pReleaseSRWLockExclusive(&srwlock_example); + ok(*u.l == 0, "expected 0x0, got %lx\n", *u.l); +} + static DWORD WINAPI alertable_wait_thread(void *param) { HANDLE *semaphores = param; @@ -2887,6 +2902,7 @@ START_TEST(sync) test_condvars_base(&unaligned_cv.cv); test_condvars_consumer_producer(); test_srwlock_base(&aligned_srwlock); + test_srwlock_quirk(); #if defined(__i386__) || defined(__x86_64__) /* unaligned locks only work on x86 platforms */ test_srwlock_base(&unaligned_srwlock.lock); diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index fa64917029a..bb5dcbb66e6 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -473,9 +473,10 @@ DWORD WINAPI RtlRunOnceExecuteOnce( RTL_RUN_ONCE *once, PRTL_RUN_ONCE_INIT_FN fu struct srw_lock { + /* bit 0 - if the lock is held exclusive. bit 1.. - number of exclusive waiters. */ short exclusive_waiters; - /* Number of shared owners, or -1 if owned exclusive. + /* Number of owners. * * Sadly Windows has no equivalent to FUTEX_WAIT_BITSET, so in order to wake * up *only* exclusive or *only* shared waiters (and thus avoid spurious @@ -486,7 +487,7 @@ struct srw_lock * the latter waits only on the "owners" member. Note then that "owners" * must not be the first element in the structure. */ - short owners; + unsigned short owners; }; C_ASSERT( sizeof(struct srw_lock) == 4 ); @@ -517,7 +518,7 @@ void WINAPI RtlAcquireSRWLockExclusive( RTL_SRWLOCK *lock ) { union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock }; - InterlockedIncrement16( &u.s->exclusive_waiters ); + InterlockedExchangeAdd16( &u.s->exclusive_waiters, 2 ); for (;;) { @@ -532,8 +533,9 @@ void WINAPI RtlAcquireSRWLockExclusive( RTL_SRWLOCK *lock ) if (!old.s.owners) { /* Not locked exclusive or shared. We can try to grab it. */ - new.s.owners = -1; - --new.s.exclusive_waiters; + new.s.owners = 1; + new.s.exclusive_waiters -= 2; + new.s.exclusive_waiters |= 1; wait = FALSE; } else @@ -568,7 +570,7 @@ void WINAPI RtlAcquireSRWLockShared( RTL_SRWLOCK *lock ) old.s = *u.s; new = old; - if (old.s.owners != -1 && !old.s.exclusive_waiters) + if (!old.s.exclusive_waiters) { /* Not locked exclusive, and no exclusive waiters. * We can try to grab it. */ @@ -599,9 +601,10 @@ void WINAPI RtlReleaseSRWLockExclusive( RTL_SRWLOCK *lock ) old.s = *u.s; new = old; - if (old.s.owners != -1) ERR("Lock %p is not owned exclusive!\n", lock); + if (!(old.s.exclusive_waiters & 1)) ERR("Lock %p is not owned exclusive!\n", lock); new.s.owners = 0; + new.s.exclusive_waiters &= ~1; } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l); if (new.s.exclusive_waiters) @@ -623,7 +626,7 @@ void WINAPI RtlReleaseSRWLockShared( RTL_SRWLOCK *lock ) old.s = *u.s; new = old; - if (old.s.owners == -1) ERR("Lock %p is owned exclusive!\n", lock); + if (old.s.exclusive_waiters & 1) ERR("Lock %p is owned exclusive!\n", lock); else if (!old.s.owners) ERR("Lock %p is not owned shared!\n", lock); --new.s.owners; @@ -654,7 +657,8 @@ BOOLEAN WINAPI RtlTryAcquireSRWLockExclusive( RTL_SRWLOCK *lock ) if (!old.s.owners) { /* Not locked exclusive or shared. We can try to grab it. */ - new.s.owners = -1; + new.s.owners = 1; + new.s.exclusive_waiters |= 1; ret = TRUE; } else @@ -680,7 +684,7 @@ BOOLEAN WINAPI RtlTryAcquireSRWLockShared( RTL_SRWLOCK *lock ) old.s = *u.s; new.s = old.s; - if (old.s.owners != -1 && !old.s.exclusive_waiters) + if (!old.s.exclusive_waiters) { /* Not locked exclusive, and no exclusive waiters. * We can try to grab it. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4310
removed problematic test case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51144
The most recent solution seems good; the fact that it even simplifies the arithmetic is particularly nice. My only request is to explicitly mention the offending application in the test, so it's clear why it's there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4310#note_51566
participants (3)
-
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Zebediah Figura (＠zfigura)