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/obj... ============================================================================== --- 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;