[ion] 25473: - Fix locking bugs in guarded mutex implementation. In race conditions some operations were not re-attempted. - Fix some other logic bugs, including a serious bug in KeTrytoAcquireGuardedMutex which inversed the result.
Author: ion Date: Tue Jan 16 00:34:36 2007 New Revision: 25473
URL: http://svn.reactos.org/svn/reactos?rev=25473&view=rev Log: - Fix locking bugs in guarded mutex implementation. In race conditions some operations were not re-attempted. - Fix some other logic bugs, including a serious bug in KeTrytoAcquireGuardedMutex which inversed the result.
Modified: trunk/reactos/ntoskrnl/include/internal/ex.h trunk/reactos/ntoskrnl/ke/gate.c trunk/reactos/ntoskrnl/ke/gmutex.c
Modified: trunk/reactos/ntoskrnl/include/internal/ex.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/e... ============================================================================== --- trunk/reactos/ntoskrnl/include/internal/ex.h (original) +++ trunk/reactos/ntoskrnl/include/internal/ex.h Tue Jan 16 00:34:36 2007 @@ -745,7 +745,8 @@ ASSERT(PushLock->Waiting || PushLock->Shared == 0);
/* Unlock the pushlock */ - OldValue.Value = InterlockedExchangeAddSizeT((PLONG)PushLock, -1); + OldValue.Value = InterlockedExchangeAddSizeT((PLONG)PushLock, + -EX_PUSH_LOCK_LOCK);
/* Sanity checks */ ASSERT(OldValue.Locked);
Modified: trunk/reactos/ntoskrnl/ke/gate.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ke/gate.c?rev=2547... ============================================================================== --- trunk/reactos/ntoskrnl/ke/gate.c (original) +++ trunk/reactos/ntoskrnl/ke/gate.c Tue Jan 16 00:34:36 2007 @@ -78,7 +78,7 @@
/* Release the APC lock and return */ KiReleaseApcLock(&ApcLock); - return; + break; }
/* Setup a Wait Block */ @@ -122,8 +122,8 @@ /* Find a new thread to run */ Status = KiSwapThread(Thread, KeGetCurrentPrcb());
- /* Check if we were executing an APC */ - if (Status != STATUS_KERNEL_APC) return; + /* Make sure we weren't executing an APC */ + if (Status == STATUS_SUCCESS) return; } } while (TRUE); } @@ -149,18 +149,14 @@ KiAcquireDispatcherObject(&Gate->Header);
/* Make sure we're not already signaled or that the list is empty */ - if (Gate->Header.SignalState) - { - /* Lower IRQL and quit */ - KeLowerIrql(OldIrql); - return; - } + if (Gate->Header.SignalState) break;
/* Check if our wait list is empty */ if (IsListEmpty(&Gate->Header.WaitListHead)) { /* It is, so signal the event */ Gate->Header.SignalState = 1; + break; } else { @@ -187,7 +183,7 @@ RemoveEntryList(&WaitBlock->WaitListEntry);
/* Clear wait status */ - WaitThread->WaitStatus = 0; + WaitThread->WaitStatus = STATUS_SUCCESS;
/* Set state and CPU */ WaitThread->State = DeferredReady; @@ -223,6 +219,7 @@
/* Exit the dispatcher */ KiExitDispatcher(OldIrql); + return; } }
Modified: trunk/reactos/ntoskrnl/ke/gmutex.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ke/gmutex.c?rev=25... ============================================================================== --- trunk/reactos/ntoskrnl/ke/gmutex.c (original) +++ trunk/reactos/ntoskrnl/ke/gmutex.c Tue Jan 16 00:34:36 2007 @@ -30,9 +30,6 @@ BitsToRemove = GM_LOCK_BIT; BitsToAdd = GM_LOCK_WAITER_INC;
- /* Get the Count Bits */ - OldValue = GuardedMutex->Count; - /* Start change loop */ for (;;) { @@ -42,43 +39,47 @@ ASSERT((BitsToAdd == GM_LOCK_WAITER_INC) || (BitsToAdd == GM_LOCK_WAITER_WOKEN));
- /* Check if the Guarded Mutex is locked */ - if (OldValue & GM_LOCK_BIT) + /* Get the Count Bits */ + OldValue = GuardedMutex->Count; + + /* Start internal bit change loop */ + for (;;) { - /* Sanity check */ - ASSERT((BitsToRemove == GM_LOCK_BIT) || - ((OldValue & GM_LOCK_WAITER_WOKEN) != 0)); - - /* Unlock it by removing the Lock Bit */ - NewValue = InterlockedCompareExchange(&GuardedMutex->Count, - OldValue ^ BitsToRemove, - OldValue); - if (NewValue == OldValue) break; - - /* Value got changed behind our backs, start over */ + /* Check if the Guarded Mutex is locked */ + if (OldValue & GM_LOCK_BIT) + { + /* Sanity check */ + ASSERT((BitsToRemove == GM_LOCK_BIT) || + ((OldValue & GM_LOCK_WAITER_WOKEN) != 0)); + + /* Unlock it by removing the Lock Bit */ + NewValue = OldValue ^ BitsToRemove; + NewValue = InterlockedCompareExchange(&GuardedMutex->Count, + NewValue, + OldValue); + if (NewValue == OldValue) return; + } + else + { + /* The Guarded Mutex isn't locked, so simply set the bits */ + NewValue = OldValue + BitsToAdd; + NewValue = InterlockedCompareExchange(&GuardedMutex->Count, + NewValue, + OldValue); + if (NewValue == OldValue) break; + } + + /* Old value changed, loop again */ OldValue = NewValue; } - else - { - /* The Guarded Mutex isn't locked, so simply set the bits */ - NewValue = InterlockedCompareExchange(&GuardedMutex->Count, - OldValue + BitsToAdd, - OldValue); - if (NewValue != OldValue) - { - /* Value got changed behind our backs, start over */ - OldValue = NewValue; - continue; - } - - /* Now we have to wait for it */ - KeWaitForGate(&GuardedMutex->Gate, WrGuardedMutex, KernelMode); - ASSERT((GuardedMutex->Count & GM_LOCK_WAITER_WOKEN) != 0); - - /* Ok, the wait is done, so set the new bits */ - BitsToRemove = GM_LOCK_BIT | GM_LOCK_WAITER_WOKEN; - BitsToAdd = GM_LOCK_WAITER_WOKEN; - } + + /* Now we have to wait for it */ + KeWaitForGate(&GuardedMutex->Gate, WrGuardedMutex, KernelMode); + ASSERT((GuardedMutex->Count & GM_LOCK_WAITER_WOKEN) != 0); + + /* Ok, the wait is done, so set the new bits */ + BitsToRemove = GM_LOCK_BIT | GM_LOCK_WAITER_WOKEN; + BitsToAdd = GM_LOCK_WAITER_WOKEN; } }
@@ -109,7 +110,7 @@ GuardedMutex->Owner = NULL;
/* Add the Lock Bit */ - OldValue = InterlockedExchangeAdd(&GuardedMutex->Count, 1); + OldValue = InterlockedExchangeAdd(&GuardedMutex->Count, GM_LOCK_BIT); ASSERT((OldValue & GM_LOCK_BIT) == 0);
/* Check if it was already locked, but not woken */ @@ -247,7 +248,7 @@
/* Remove the lock */ OldBit = InterlockedBitTestAndReset(&GuardedMutex->Count, GM_LOCK_BIT_V); - if (OldBit) + if (!OldBit) { /* Re-enable APCs */ KeLeaveGuardedRegion();
-
ion＠svn.reactos.org