[ros-diffs] [tkreuzer] 39629: gdiobj.c: go back from intrinsics to to portable interlocked functions. InterlockedPopFreeEntry: Use InterlockedIncrement instead of InterlockedCompareExchange when exchanging the next unused entry in the handle table. Lock the free entry before accessing it, fixing a race condition. Might fix bug #4115 See issue #4115 for more details.

Author: tkreuzer
Date: Mon Feb 16 17:00:15 2009
New Revision: 39629
URL: http://svn.reactos.org/svn/reactos?rev=39629&view=rev
Log:
gdiobj.c: go back from intrinsics to to portable interlocked functions.
InterlockedPopFreeEntry: Use InterlockedIncrement instead of InterlockedCompareExchange
when exchanging the next unused entry in the handle table. Lock the free entry before
accessing it, fixing a race condition. Might fix bug #4115
See issue #4115 for more details.
Modified:
    trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c
Modified: trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ob…
==============================================================================
--- trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c [iso-8859-1] (original)
+++ trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c [iso-8859-1] Mon Feb 16
17:00:15 2009
@@ -198,36 +198,64 @@
 FASTCALL
 InterlockedPopFreeEntry()
 {
-    ULONG idxFirstFree, idxNextFree, idxPrev;
-    PGDI_TABLE_ENTRY pFreeEntry;
+    ULONG idxFirst, idxNext, idxPrev;
+    PGDI_TABLE_ENTRY pEntry;
+    DWORD PrevProcId;
     DPRINT("Enter InterLockedPopFreeEntry\n");
-    do
-    {
-        idxFirstFree = GdiHandleTable->FirstFree;
-        if (idxFirstFree)
-        {
-            pFreeEntry = GdiHandleTable->Entries + idxFirstFree;
-            ASSERT(((ULONG)pFreeEntry->KernelData & ~GDI_HANDLE_INDEX_MASK) == 0);
-            idxNextFree = (ULONG)pFreeEntry->KernelData;
-            idxPrev =
(ULONG)_InterlockedCompareExchange((LONG*)&GdiHandleTable->FirstFree, idxNextFree,
idxFirstFree);
-        }
-        else
-        {
-            idxFirstFree = GdiHandleTable->FirstUnused;
-            idxNextFree = idxFirstFree + 1;
-            if (idxNextFree >= GDI_HANDLE_COUNT)
+    while (TRUE)
+    {
+        idxFirst = GdiHandleTable->FirstFree;
+
+        if (!idxFirst)
+        {
+            /* Increment FirstUnused and get the new index */
+            idxFirst = InterlockedIncrement((LONG*)&GdiHandleTable->FirstUnused) -
1;
+
+            /* Check if we have entries left */
+            if (idxFirst >= GDI_HANDLE_COUNT)
             {
                 DPRINT1("No more gdi handles left!\n");
                 return 0;
             }
-            idxPrev =
(ULONG)_InterlockedCompareExchange((LONG*)&GdiHandleTable->FirstUnused,
idxNextFree, idxFirstFree);
-        }
-    }
-    while (idxPrev != idxFirstFree);
-
-    return idxFirstFree;
+
+            /* Return the old index */
+            return idxFirst;
+        }
+
+        /* Get a pointer to the first free entry */
+        pEntry = GdiHandleTable->Entries + idxFirst;
+
+        /* Try to lock the entry */
+        PrevProcId = InterlockedCompareExchange((LONG*)&pEntry->ProcessId, 1, 0);
+        if (PrevProcId != 0)
+        {
+            /* The entry was locked or not free, wait and start over */
+            DelayExecution();
+            continue;
+        }
+
+        /* Sanity check: is entry really free? */
+        ASSERT(((ULONG_PTR)pEntry->KernelData & ~GDI_HANDLE_INDEX_MASK) == 0);
+
+        /* Try to exchange the FirstFree value */
+        idxNext = (ULONG_PTR)pEntry->KernelData;
+        idxPrev = InterlockedCompareExchange((LONG*)&GdiHandleTable->FirstFree,
+                                             idxNext,
+                                             idxFirst);
+
+        /* Unlock the free entry */
+        (void)InterlockedExchange((LONG*)&pEntry->ProcessId, 0);
+
+        /* If we succeeded, break out of the loop */
+        if (idxPrev == idxFirst)
+        {
+            break;
+        }
+    }
+
+    return idxFirst;
 }
 /* Pushes an entry of the handle table to the free list,
@@ -249,9 +277,11 @@
     do
     {
         idxFirstFree = GdiHandleTable->FirstFree;
-        pFreeEntry->KernelData = (PVOID)idxFirstFree;
-
-        idxPrev =
(ULONG)_InterlockedCompareExchange((LONG*)&GdiHandleTable->FirstFree, idxToFree,
idxFirstFree);
+        pFreeEntry->KernelData = (PVOID)(ULONG_PTR)idxFirstFree;
+
+        idxPrev = InterlockedCompareExchange((LONG*)&GdiHandleTable->FirstFree,
+                                             idxToFree,
+                                             idxFirstFree);
     }
     while (idxPrev != idxFirstFree);
 }
@@ -365,7 +395,7 @@
         Entry = &GdiHandleTable->Entries[Index];
 LockHandle:
-        PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, 0);
+        PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, 0);
         if (PrevProcId == NULL)
         {
             PW32THREAD Thread = (PW32THREAD)PsGetCurrentThreadWin32Thread();
@@ -390,13 +420,13 @@
             newObject->Tid = Thread;
             /* unlock the entry */
-            (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
CurrentProcessId);
+            (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
CurrentProcessId);
             GDIDBG_CAPTUREALLOCATOR(Index);
             if (W32Process != NULL)
             {
-                _InterlockedIncrement(&W32Process->GDIObjects);
+                InterlockedIncrement(&W32Process->GDIObjects);
             }
             DPRINT("GDIOBJ_AllocObj: 0x%x ob: 0x%x\n", Handle, newObject);
@@ -496,7 +526,7 @@
 LockHandle:
     /* lock the object, we must not delete global objects, so don't exchange the
locking
        process ID to zero when attempting to lock a global object... */
-    PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, ProcessId);
+    PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, ProcessId);
     if (PrevProcId == ProcessId)
     {
         if ( (Entry->KernelData != NULL) &&
@@ -516,7 +546,7 @@
                 Entry->Type = (Entry->Type + GDI_ENTRY_REUSE_INC) &
~GDI_ENTRY_BASETYPE_MASK;
                 /* unlock the handle slot */
-                (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
NULL);
+                (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId, NULL);
                 /* push this entry to the free list */
                 InterlockedPushFreeEntry(GDI_ENTRY_TO_INDEX(GdiHandleTable, Entry));
@@ -525,7 +555,7 @@
                 if (W32Process != NULL)
                 {
-                    _InterlockedDecrement(&W32Process->GDIObjects);
+                    InterlockedDecrement(&W32Process->GDIObjects);
                 }
                 /* call the cleanup routine. */
@@ -546,7 +576,7 @@
                 DPRINT1("Object->cExclusiveLock = %d\n",
Object->cExclusiveLock);
                 GDIDBG_TRACECALLER();
                 GDIDBG_TRACELOCKER(GDI_HANDLE_GET_INDEX(hObj));
-                (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+                (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
                 /* do not assert here for it will call again from dxg.sys it being call
twice */
                 //ASSERT(FALSE);
             }
@@ -554,7 +584,7 @@
         else
         {
             LockErrorDebugOutput(hObj, Entry, "GDIOBJ_FreeObj");
-            (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+            (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
         }
     }
     else if (PrevProcId == LockedProcessId)
@@ -791,7 +821,7 @@
     {
         /* Lock the handle table entry. */
         LockedProcessId = (HANDLE)((ULONG_PTR)HandleProcessId | 0x1);
-        PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
+        PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
                                                         LockedProcessId,
                                                         HandleProcessId);
@@ -819,12 +849,12 @@
                     if (Object->Tid != Thread)
                     {
                         /* Unlock the handle table entry. */
-
(void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId, PrevProcId);
+
(void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId, PrevProcId);
                         DelayExecution();
                         continue;
                     }
-                    _InterlockedIncrement((PLONG)&Object->cExclusiveLock);
+                    InterlockedIncrement((PLONG)&Object->cExclusiveLock);
                 }
             }
             else
@@ -837,7 +867,7 @@
             }
             /* Unlock the handle table entry. */
-            (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+            (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
             break;
         }
@@ -924,7 +954,7 @@
     {
         /* Lock the handle table entry. */
         LockedProcessId = (HANDLE)((ULONG_PTR)HandleProcessId | 0x1);
-        PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
+        PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
                                                         LockedProcessId,
                                                         HandleProcessId);
@@ -941,13 +971,13 @@
                 Object = (POBJ)Entry->KernelData;
 #ifdef GDI_DEBUG
-                if (_InterlockedIncrement((PLONG)&Object->ulShareCount) == 1)
+                if (InterlockedIncrement((PLONG)&Object->ulShareCount) == 1)
                 {
                     memset(GDIHandleLocker[HandleIndex], 0x00, GDI_STACK_LEVELS *
sizeof(ULONG));
                     RtlCaptureStackBackTrace(1, GDI_STACK_LEVELS,
(PVOID*)GDIHandleLocker[HandleIndex], NULL);
                 }
 #else
-                _InterlockedIncrement((PLONG)&Object->ulShareCount);
+                InterlockedIncrement((PLONG)&Object->ulShareCount);
 #endif
             }
             else
@@ -960,7 +990,7 @@
             }
             /* Unlock the handle table entry. */
-            (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+            (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
             break;
         }
@@ -990,7 +1020,7 @@
 VOID INTERNAL_CALL
 GDIOBJ_UnlockObjByPtr(POBJ Object)
 {
-    if (_InterlockedDecrement((PLONG)&Object->cExclusiveLock) < 0)
+    if (InterlockedDecrement((PLONG)&Object->cExclusiveLock) < 0)
     {
         DPRINT1("Trying to unlock non-existant object\n");
     }
@@ -999,7 +1029,7 @@
 VOID INTERNAL_CALL
 GDIOBJ_ShareUnlockObjByPtr(POBJ Object)
 {
-    if (_InterlockedDecrement((PLONG)&Object->ulShareCount) < 0)
+    if (InterlockedDecrement((PLONG)&Object->ulShareCount) < 0)
     {
         DPRINT1("Trying to unlock non-existant object\n");
     }
@@ -1059,7 +1089,7 @@
 LockHandle:
         /* lock the object, we must not convert stock objects, so don't check!!! */
-        PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, ProcessId);
+        PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
LockedProcessId, ProcessId);
         if (PrevProcId == ProcessId)
         {
             LONG NewType, PrevType, OldType;
@@ -1079,7 +1109,7 @@
             NewType = OldType | GDI_ENTRY_STOCK_MASK;
             /* Try to exchange the type field - but only if the old (previous type)
matches! */
-            PrevType = _InterlockedCompareExchange(&Entry->Type, NewType,
OldType);
+            PrevType = InterlockedCompareExchange(&Entry->Type, NewType, OldType);
             if (PrevType == OldType && Entry->KernelData != NULL)
             {
                 PW32THREAD PrevThread;
@@ -1108,7 +1138,7 @@
                             W32Process = (PW32PROCESS)OldProcess->Win32Process;
                             if (W32Process != NULL)
                             {
-                                _InterlockedDecrement(&W32Process->GDIObjects);
+                                InterlockedDecrement(&W32Process->GDIObjects);
                             }
                             ObDereferenceObject(OldProcess);
                         }
@@ -1119,7 +1149,7 @@
                     Object->hHmgr = hObj;
                     /* remove the process id lock and make it global */
-                    (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
GDI_GLOBAL_PROCESS);
+                    (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
GDI_GLOBAL_PROCESS);
                     /* we're done, successfully converted the object */
                     return TRUE;
@@ -1131,7 +1161,7 @@
                     /* WTF?! The object is already locked by a different thread!
                        Release the lock, wait a bit and try again!
                        FIXME - we should give up after some time unless we want to wait
forever! */
-                    (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+                    (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
                     DelayExecution();
                     goto LockHandle;
@@ -1185,7 +1215,7 @@
 LockHandle:
         /* lock the object, we must not convert stock objects, so don't check!!! */
-        PrevProcId = _InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
ProcessId, LockedProcessId);
+        PrevProcId = InterlockedCompareExchangePointer((PVOID*)&Entry->ProcessId,
ProcessId, LockedProcessId);
         if (PrevProcId == ProcessId)
         {
             PW32THREAD PrevThread;
@@ -1211,7 +1241,7 @@
                             W32Process = (PW32PROCESS)OldProcess->Win32Process;
                             if (W32Process != NULL)
                             {
-                                _InterlockedDecrement(&W32Process->GDIObjects);
+                                InterlockedDecrement(&W32Process->GDIObjects);
                             }
                             ObDereferenceObject(OldProcess);
                         }
@@ -1225,14 +1255,14 @@
                         W32Process = (PW32PROCESS)NewOwner->Win32Process;
                         if (W32Process != NULL)
                         {
-                            _InterlockedIncrement(&W32Process->GDIObjects);
+                            InterlockedIncrement(&W32Process->GDIObjects);
                         }
                     }
                     else
                         ProcessId = 0;
                     /* remove the process id lock and change it to the new process id */
-                    (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
ProcessId);
+                    (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
ProcessId);
                     /* we're done! */
                     return Ret;
@@ -1247,7 +1277,7 @@
                        being deleted in the meantime (because we don't have aquired a
reference
                        at this point).
                        FIXME - we should give up after some time unless we want to wait
forever! */
-                    (void)_InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
+                    (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId,
PrevProcId);
                     DelayExecution();
                     goto LockHandle;
@@ -1314,7 +1344,7 @@
 LockHandleFrom:
         /* lock the object, we must not convert stock objects, so don't check!!! */
-        FromPrevProcId =
_InterlockedCompareExchangePointer((PVOID*)&FromEntry->ProcessId, FromProcessId,
FromLockedProcessId);
+        FromPrevProcId =
InterlockedCompareExchangePointer((PVOID*)&FromEntry->ProcessId, FromProcessId,
FromLockedProcessId);
         if (FromPrevProcId == FromProcessId)
         {
             PW32THREAD PrevThread;
@@ -1347,7 +1377,7 @@
                         GDIOBJ_SetOwnership(CopyTo, NULL);
                     }
-
(void)_InterlockedExchangePointer((PVOID*)&FromEntry->ProcessId, FromPrevProcId);
+
(void)InterlockedExchangePointer((PVOID*)&FromEntry->ProcessId, FromPrevProcId);
                 }
                 else
                 {
@@ -1359,7 +1389,7 @@
                        being deleted in the meantime (because we don't have aquired a
reference
                        at this point).
                        FIXME - we should give up after some time unless we want to wait
forever! */
-
(void)_InterlockedExchangePointer((PVOID*)&FromEntry->ProcessId, FromPrevProcId);
+
(void)InterlockedExchangePointer((PVOID*)&FromEntry->ProcessId, FromPrevProcId);
                     DelayExecution();
                     goto LockHandleFrom;