[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/…
==============================================================================
--- 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();