Fix CRWLock ID generation overflow bug
authorKoundinya Veluri <kouvel@microsoft.com>
Wed, 17 Aug 2016 21:48:59 +0000 (14:48 -0700)
committerKoundinya Veluri <kouvel@microsoft.com>
Wed, 17 Aug 2016 21:48:59 +0000 (14:48 -0700)
commit0a0b67c1fb89ed0e9d496063815f13d3ee55dcaa
tree0e20cf665122963dfb1598860cb97073e492c9b6
parent3b46c5ae24325d7b31efedf4ed5435c4cb0f43e4
Fix CRWLock ID generation overflow bug

There are several problems with the CRWLock constructor, where it tries to increment a 64-bit lock ID in two 32-bit pieces using lock-free code in a fast path:
- For the lower 32 bits, LLockID, zero is reserved for identifying a free lock entry. Upon incrementing LLockID from -1 to 0, the code was not skipping zero or incrementing the upper 32 bits, ULockID. Instead, it waited for one more increment to LLockID before increment ULockID. Assignment of the invalid LLockID caused a free lock entry to be reused for a new lock, and corruption in lock state thereafter. This is the issue identified by the bug.
- If LLockID != -1 in thread A, but the compare-exchange fails because thread B had updated the value before thread A, ULockID was not being read again in thread A. If thread B had also incremented ULockID, thread A would continue to use the old ULockID.
- In the locked slow path that handles incrementing ULockID, ULockID is updated before LLockID. The constructor though, was reading ULockID before LLockID. This allows a race where Thread A reads an old value for ULockID and a new value for LLockID, increments LLockID successfully, and continues to use the old ULockID.

Due to the availability of InterlockedCompareExchange64 on all supported platforms, we decided to use that instead to simplify the solution and fix all of the above issues at the same time.

Bug: 242568
Integrated from changes: 1621197, 1621810

[tfs-changeset: 1622814]

Commit migrated from https://github.com/dotnet/coreclr/commit/6ad3d45d6b9096b64c9d9ea933efe5f49f08c81f
src/coreclr/src/vm/rwlock.cpp
src/coreclr/src/vm/rwlock.h