[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.
tkreuzer at svn.reactos.org
tkreuzer at svn.reactos.org
Tue Feb 17 00:00:16 CET 2009
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/objects/gdiobj.c?rev=39629&r1=39628&r2=39629&view=diff
==============================================================================
--- 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;
More information about the Ros-diffs
mailing list