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.
-- v8: ntdll: Tweak the binary representation of SRWLOCK.
From: Yuxuan Shui yshui@codeweavers.com
--- include/winnt.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/winnt.h b/include/winnt.h index 7bbf224e3c9..fb011f99d99 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); +long _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 LONG 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 );
From: Yuxuan Shui yshui@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 | 21 +++++++++++++++++++++ dlls/ntdll/sync.c | 23 ++++++++++++++--------- 2 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c index 10765765bc5..13fa0231bf7 100644 --- a/dlls/kernel32/tests/sync.c +++ b/dlls/kernel32/tests/sync.c @@ -2535,6 +2535,26 @@ 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); + + *u.l = 0; + pReleaseSRWLockExclusive(&srwlock_example); + todo_wine ok(*u.l == 0xffffffff, "expected 0xffffffff, got %lx\n", *u.l); + +} + static DWORD WINAPI alertable_wait_thread(void *param) { HANDLE *semaphores = param; @@ -2887,6 +2907,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..3d7cf9f777c 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 @@ -487,6 +488,7 @@ struct srw_lock * must not be the first element in the structure. */ short owners; + }; C_ASSERT( sizeof(struct srw_lock) == 4 );
@@ -517,7 +519,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 +534,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 +571,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 +602,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 +627,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 +658,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 +685,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. */
update: use a different approach that doesn't require `owners` to be the first field.
-
Yuxuan Shui -
Yuxuan Shui (＠yshui)