15 Jan
2007
15 Jan
'07
9:34 p.m.
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/…
==============================================================================
--- 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=254…
==============================================================================
--- 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=2…
==============================================================================
--- 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();