[ros-diffs] [tkreuzer] 27818: gdiobjects patch: - stockmask and reuse counter are stored in the lower 16 bits of the typeinfo field in the entry, not in the upper 16 bits! - take care of this when trying to lock an object (compare upper 16 bits of handle with lower 16 bits of typeinfo field) - some variable renaming - remove code duplication by moving debugoutput to an extra function - move checking for expected object type to a different location, should be removed completely later Fixes possible crashes if messing around with deleted / reused objects.
tkreuzer at svn.reactos.org
tkreuzer at svn.reactos.org
Thu Jul 26 00:39:15 CEST 2007
Author: tkreuzer
Date: Thu Jul 26 02:39:14 2007
New Revision: 27818
URL: http://svn.reactos.org/svn/reactos?rev=27818&view=rev
Log:
gdiobjects patch:
- stockmask and reuse counter are stored in the lower 16 bits of the typeinfo field in the entry, not in the upper 16 bits!
- take care of this when trying to lock an object (compare upper 16 bits of handle with lower 16 bits of typeinfo field)
- some variable renaming
- remove code duplication by moving debugoutput to an extra function
- move checking for expected object type to a different location, should be removed completely later
Fixes possible crashes if messing around with deleted / reused objects.
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=27818&r1=27817&r2=27818&view=diff
==============================================================================
--- trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c (original)
+++ trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c Thu Jul 26 02:39:14 2007
@@ -323,6 +323,33 @@
}
#endif /* GDI_DEBUG */
+
+static void FASTCALL
+LockErrorDebugOutput(HGDIOBJ hObj, PGDI_TABLE_ENTRY Entry, LPSTR Function)
+{
+ if (Entry->KernelData == NULL)
+ {
+ DPRINT1("%s: Attempted to lock object 0x%x that is deleted!\n", Function, hObj);
+ }
+ else if (GDI_HANDLE_GET_REUSECNT(hObj) != GDI_ENTRY_GET_REUSECNT(Entry->Type))
+ {
+ DPRINT1("%s: Attempted to lock object 0x%x, wrong reuse counter (Handle: 0x%x, Entry: 0x%x)\n",
+ Function, hObj, GDI_HANDLE_GET_REUSECNT(hObj), GDI_ENTRY_GET_REUSECNT(Entry->Type));
+ }
+ else if (GDI_HANDLE_GET_TYPE(hObj) != GDI_HANDLE_GET_TYPE(Entry->Type))
+ {
+ DPRINT1("%s: Attempted to lock object 0x%x, type mismatch (Handle: 0x%x, Entry: 0x%x)\n",
+ Function, hObj, GDI_HANDLE_GET_TYPE(hObj), GDI_HANDLE_GET_TYPE(Entry->Type));
+ }
+ else
+ {
+ DPRINT1("%s: Attempted to lock object 0x%x, something went wrong, typeinfo = 0x%x\n",
+ Function, hObj, Entry->Type);
+ }
+ KeRosDumpStackFrames(NULL, 20);
+
+}
+
/*!
* Allocate memory for GDI object and return handle to it.
*
@@ -384,7 +411,10 @@
RtlZeroMemory(ObjectBody, GetObjectSize(ObjectType));
- TypeInfo = (ObjectType & GDI_HANDLE_TYPE_MASK) | (ObjectType >> 16);
+ /* FIXME: On Windows the higher 16 bit of the type field don't always match
+ the type from the handle, it is probably a storage type
+ (type = pen, storage = brush) */
+ TypeInfo = (ObjectType & GDI_HANDLE_TYPE_MASK) | (ObjectType >> GDI_ENTRY_UPPER_SHIFT);
FreeEntry = InterlockedPopEntrySList(&HandleTable->FreeEntriesHead);
if(FreeEntry != NULL)
@@ -408,7 +438,7 @@
Entry->KernelData = ObjectBody;
/* copy the reuse-counter */
- TypeInfo |= Entry->Type & GDI_HANDLE_REUSE_MASK;
+ TypeInfo |= Entry->Type & GDI_ENTRY_REUSE_MASK;
/* we found a free entry, no need to exchange this field atomically
since we're holding the lock */
@@ -426,7 +456,7 @@
{
InterlockedIncrement(&W32Process->GDIObjects);
}
- Handle = (HGDIOBJ)((Index & 0xFFFF) | (TypeInfo & (GDI_HANDLE_TYPE_MASK | GDI_HANDLE_REUSE_MASK)));
+ Handle = (HGDIOBJ)((Index & 0xFFFF) | (TypeInfo << GDI_ENTRY_UPPER_SHIFT));
DPRINT("GDIOBJ_AllocObj: 0x%x ob: 0x%x\n", Handle, ObjectBody);
return Handle;
@@ -478,15 +508,15 @@
*/
BOOL INTERNAL_CALL
#ifdef GDI_DEBUG
-GDIOBJ_FreeObjDbg(PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_FreeObjDbg(PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ExpectedType)
#else /* !GDI_DEBUG */
-GDIOBJ_FreeObj(PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_FreeObj(PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ExpectedType)
#endif /* GDI_DEBUG */
{
PGDI_TABLE_ENTRY Entry;
PPAGED_LOOKASIDE_LIST LookasideList;
HANDLE ProcessId, LockedProcessId, PrevProcId;
- LONG ExpectedType;
+ ULONG HandleType, HandleUpper;
BOOL Silent;
#ifdef GDI_DEBUG
ULONG Attempts = 0;
@@ -506,10 +536,21 @@
ProcessId = PsGetCurrentProcessId();
LockedProcessId = (HANDLE)((ULONG_PTR)ProcessId | 0x1);
- Silent = (ObjectType & GDI_OBJECT_TYPE_SILENT);
- ObjectType &= ~GDI_OBJECT_TYPE_SILENT;
-
- ExpectedType = ((ObjectType != GDI_OBJECT_TYPE_DONTCARE) ? ObjectType : 0);
+ Silent = (ExpectedType & GDI_OBJECT_TYPE_SILENT);
+ ExpectedType &= ~GDI_OBJECT_TYPE_SILENT;
+
+ HandleType = GDI_HANDLE_GET_TYPE(hObj);
+ HandleUpper = GDI_HANDLE_GET_UPPER(hObj);
+
+ /* Check if we have the requested type */
+ if ( (ExpectedType != GDI_OBJECT_TYPE_DONTCARE &&
+ HandleType != ExpectedType) ||
+ HandleType == 0 )
+ {
+ DPRINT1("Attempted to free object 0x%x of wrong type (Handle: 0x%x, expected: 0x%x)\n",
+ hObj, HandleType, ExpectedType);
+ return FALSE;
+ }
Entry = GDI_HANDLE_GET_ENTRY(HandleTable, hObj);
@@ -519,10 +560,8 @@
PrevProcId = InterlockedCompareExchangePointer(&Entry->ProcessId, LockedProcessId, ProcessId);
if(PrevProcId == ProcessId)
{
- if(Entry->Type != 0 && Entry->KernelData != NULL &&
- (ExpectedType == 0 || ((Entry->Type << 16) == ExpectedType)) &&
- (Entry->Type & (GDI_HANDLE_TYPE_MASK | GDI_HANDLE_REUSE_MASK)) ==
- ((ULONG_PTR)hObj & (GDI_HANDLE_TYPE_MASK | GDI_HANDLE_REUSE_MASK)))
+ if( (Entry->KernelData != NULL) &&
+ ((Entry->Type << GDI_ENTRY_UPPER_SHIFT) == HandleUpper) )
{
PGDIOBJHDR GdiHdr;
@@ -532,10 +571,9 @@
{
BOOL Ret;
PW32PROCESS W32Process = PsGetCurrentProcessWin32Process();
- ULONG Type = Entry->Type << 16;
/* Clear the type field so when unlocking the handle it gets finally deleted and increment reuse counter */
- Entry->Type = ((Entry->Type >> GDI_HANDLE_REUSECNT_SHIFT) + 1) << GDI_HANDLE_REUSECNT_SHIFT;
+ Entry->Type = (Entry->Type + GDI_ENTRY_REUSE_INC) & GDI_ENTRY_REUSE_MASK;
Entry->KernelData = NULL;
/* unlock the handle slot */
@@ -551,10 +589,10 @@
}
/* call the cleanup routine. */
- Ret = RunCleanupCallback(GDIHdrToBdy(GdiHdr), Type);
+ Ret = RunCleanupCallback(GDIHdrToBdy(GdiHdr), HandleType);
/* Now it's time to free the memory */
- LookasideList = FindLookasideList(HandleTable, Type);
+ LookasideList = FindLookasideList(HandleTable, HandleType);
if(LookasideList != NULL)
{
ExFreeToPagedLookasideList(LookasideList, GdiHdr);
@@ -576,16 +614,7 @@
}
else
{
- if((Entry->Type & ~GDI_HANDLE_REUSE_MASK) != 0)
- {
- DPRINT1("Attempted to delete object 0x%x, type mismatch (0x%x : 0x%x)\n", hObj, ObjectType, ExpectedType);
- KeRosDumpStackFrames(NULL, 20);
- }
- else
- {
- DPRINT1("Attempted to delete object 0x%x which was already deleted!\n", hObj);
- KeRosDumpStackFrames(NULL, 20);
- }
+ LockErrorDebugOutput(hObj, Entry, "GDIOBJ_FreeObj");
(void)InterlockedExchangePointer(&Entry->ProcessId, PrevProcId);
}
}
@@ -678,11 +707,11 @@
{
HGDIOBJ ObjectHandle;
- /* Create the object handle for the entry, the upper 16 bit of the
+ /* Create the object handle for the entry, the lower(!) 16 bit of the
Type field includes the type of the object including the stock
object flag - but since stock objects don't have a process id we can
simply ignore this fact here. */
- ObjectHandle = (HGDIOBJ)(Index | (Entry->Type & 0xFFFF0000));
+ ObjectHandle = (HGDIOBJ)(Index | (Entry->Type << GDI_ENTRY_UPPER_SHIFT));
if(GDIOBJ_FreeObj(HandleTable, ObjectHandle, GDI_OBJECT_TYPE_DONTCARE) &&
W32Process->GDIObjects == 0)
@@ -712,30 +741,43 @@
*
* \note Process can only get pointer to the objects it created or global objects.
*
- * \todo Get rid of the ObjectType parameter!
+ * \todo Get rid of the ExpectedType parameter!
*/
PGDIOBJ INTERNAL_CALL
#ifdef GDI_DEBUG
-GDIOBJ_LockObjDbg (PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_LockObjDbg (PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ExpectedType)
#else /* !GDI_DEBUG */
-GDIOBJ_LockObj (PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_LockObj (PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ExpectedType)
#endif /* GDI_DEBUG */
{
USHORT HandleIndex;
- PGDI_TABLE_ENTRY HandleEntry;
+ PGDI_TABLE_ENTRY Entry;
HANDLE ProcessId, HandleProcessId, LockedProcessId, PrevProcId;
PGDIOBJ Object = NULL;
+ ULONG HandleType, HandleUpper;
HandleIndex = GDI_HANDLE_GET_INDEX(hObj);
+ HandleType = GDI_HANDLE_GET_TYPE(hObj);
+ HandleUpper = GDI_HANDLE_GET_UPPER(hObj);
/* Check that the handle index is valid. */
if (HandleIndex >= GDI_HANDLE_COUNT)
return NULL;
- HandleEntry = &HandleTable->Entries[HandleIndex];
+ Entry = &HandleTable->Entries[HandleIndex];
+
+ /* Check if we have the requested type */
+ if ( (ExpectedType != GDI_OBJECT_TYPE_DONTCARE &&
+ HandleType != ExpectedType) ||
+ HandleType == 0 )
+ {
+ DPRINT1("Attempted to lock object 0x%x of wrong type (Handle: 0x%x, requested: 0x%x)\n",
+ hObj, HandleType, ExpectedType);
+ return NULL;
+ }
ProcessId = (HANDLE)((ULONG_PTR)PsGetCurrentProcessId() & ~1);
- HandleProcessId = (HANDLE)((ULONG_PTR)HandleEntry->ProcessId & ~1);
+ HandleProcessId = (HANDLE)((ULONG_PTR)Entry->ProcessId & ~1);
/* Check for invalid owner. */
if (ProcessId != HandleProcessId && HandleProcessId != NULL)
@@ -760,25 +802,21 @@
{
/* Lock the handle table entry. */
LockedProcessId = (HANDLE)((ULONG_PTR)HandleProcessId | 0x1);
- PrevProcId = InterlockedCompareExchangePointer(&HandleEntry->ProcessId,
+ PrevProcId = InterlockedCompareExchangePointer(&Entry->ProcessId,
LockedProcessId,
HandleProcessId);
if (PrevProcId == HandleProcessId)
{
- LONG HandleType = HandleEntry->Type << 16;
-
/*
* We're locking an object that belongs to our process or it's a
* global object if HandleProcessId is 0 here.
*/
- /* FIXME: Check the upper 16-bits of handle number! */
- if (HandleType != 0 && HandleEntry->KernelData != NULL &&
- (ObjectType == GDI_OBJECT_TYPE_DONTCARE ||
- HandleType == ObjectType))
+ if ( (Entry->KernelData != NULL) &&
+ ((Entry->Type << GDI_ENTRY_UPPER_SHIFT) == HandleUpper) )
{
- PGDIOBJHDR GdiHdr = GDIBdyToHdr(HandleEntry->KernelData);
+ PGDIOBJHDR GdiHdr = GDIBdyToHdr(Entry->KernelData);
PETHREAD Thread = PsGetCurrentThread();
if (GdiHdr->Locks == 0)
@@ -789,7 +827,7 @@
GdiHdr->lockfile = file;
GdiHdr->lockline = line;
#endif
- Object = HandleEntry->KernelData;
+ Object = Entry->KernelData;
}
else
{
@@ -799,12 +837,12 @@
InterlockedDecrement((PLONG)&GdiHdr->Locks);
/* Unlock the handle table entry. */
- (void)InterlockedExchangePointer(&HandleEntry->ProcessId, PrevProcId);
+ (void)InterlockedExchangePointer(&Entry->ProcessId, PrevProcId);
DelayExecution();
continue;
}
- Object = HandleEntry->KernelData;
+ Object = Entry->KernelData;
}
}
else
@@ -813,26 +851,15 @@
* Debugging code. Report attempts to lock deleted handles and
* locking type mismatches.
*/
-
- if ((HandleType & ~GDI_HANDLE_REUSE_MASK) == 0)
- {
- DPRINT1("Attempted to lock object 0x%x that is deleted!\n", hObj);
- KeRosDumpStackFrames(NULL, 20);
- }
- else
- {
- DPRINT1("Attempted to lock object 0x%x, type mismatch (0x%x : 0x%x)\n",
- hObj, HandleType & ~GDI_HANDLE_REUSE_MASK, ObjectType & ~GDI_HANDLE_REUSE_MASK);
-
- KeRosDumpStackFrames(NULL, 20);
- }
+ LockErrorDebugOutput(hObj, Entry, "GDIOBJ_LockObj");
+
#ifdef GDI_DEBUG
DPRINT1("-> called from %s:%i\n", file, line);
#endif
}
/* Unlock the handle table entry. */
- (void)InterlockedExchangePointer(&HandleEntry->ProcessId, PrevProcId);
+ (void)InterlockedExchangePointer(&Entry->ProcessId, PrevProcId);
break;
}
@@ -862,30 +889,43 @@
*
* \note Process can only get pointer to the objects it created or global objects.
*
- * \todo Get rid of the ObjectType parameter!
+ * \todo Get rid of the ExpectedType parameter!
*/
PGDIOBJ INTERNAL_CALL
#ifdef GDI_DEBUG
-GDIOBJ_ShareLockObjDbg (PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_ShareLockObjDbg (PGDI_HANDLE_TABLE HandleTable, const char* file, int line, HGDIOBJ hObj, DWORD ExpectedType)
#else /* !GDI_DEBUG */
-GDIOBJ_ShareLockObj (PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ObjectType)
+GDIOBJ_ShareLockObj (PGDI_HANDLE_TABLE HandleTable, HGDIOBJ hObj, DWORD ExpectedType)
#endif /* GDI_DEBUG */
{
USHORT HandleIndex;
- PGDI_TABLE_ENTRY HandleEntry;
+ PGDI_TABLE_ENTRY Entry;
HANDLE ProcessId, HandleProcessId, LockedProcessId, PrevProcId;
PGDIOBJ Object = NULL;
+ ULONG_PTR HandleType, HandleUpper;
HandleIndex = GDI_HANDLE_GET_INDEX(hObj);
+ HandleType = GDI_HANDLE_GET_TYPE(hObj);
+ HandleUpper = GDI_HANDLE_GET_UPPER(hObj);
/* Check that the handle index is valid. */
if (HandleIndex >= GDI_HANDLE_COUNT)
return NULL;
- HandleEntry = &HandleTable->Entries[HandleIndex];
+ /* Check if we have the requested type */
+ if ( (ExpectedType != GDI_OBJECT_TYPE_DONTCARE &&
+ HandleType != ExpectedType) ||
+ HandleType == 0 )
+ {
+ DPRINT1("Attempted to lock object 0x%x of wrong type (Handle: 0x%x, requested: 0x%x)\n",
+ hObj, HandleType, ExpectedType);
+ return NULL;
+ }
+
+ Entry = &HandleTable->Entries[HandleIndex];
ProcessId = (HANDLE)((ULONG_PTR)PsGetCurrentProcessId() & ~1);
- HandleProcessId = (HANDLE)((ULONG_PTR)HandleEntry->ProcessId & ~1);
+ HandleProcessId = (HANDLE)((ULONG_PTR)Entry->ProcessId & ~1);
/* Check for invalid owner. */
if (ProcessId != HandleProcessId && HandleProcessId != NULL)
@@ -910,25 +950,21 @@
{
/* Lock the handle table entry. */
LockedProcessId = (HANDLE)((ULONG_PTR)HandleProcessId | 0x1);
- PrevProcId = InterlockedCompareExchangePointer(&HandleEntry->ProcessId,
+ PrevProcId = InterlockedCompareExchangePointer(&Entry->ProcessId,
LockedProcessId,
HandleProcessId);
if (PrevProcId == HandleProcessId)
{
- LONG HandleType = HandleEntry->Type << 16;
-
/*
* We're locking an object that belongs to our process or it's a
* global object if HandleProcessId is 0 here.
*/
- /* FIXME: Check the upper 16-bits of handle number! */
- if (HandleType != 0 && HandleEntry->KernelData != NULL &&
- (ObjectType == GDI_OBJECT_TYPE_DONTCARE ||
- HandleType == ObjectType))
+ if ( (Entry->KernelData != NULL) &&
+ (HandleUpper == (Entry->Type << GDI_ENTRY_UPPER_SHIFT)) )
{
- PGDIOBJHDR GdiHdr = GDIBdyToHdr(HandleEntry->KernelData);
+ PGDIOBJHDR GdiHdr = GDIBdyToHdr(Entry->KernelData);
#ifdef GDI_DEBUG
if (InterlockedIncrement((PLONG)&GdiHdr->Locks) == 1)
@@ -939,7 +975,7 @@
#else
InterlockedIncrement((PLONG)&GdiHdr->Locks);
#endif
- Object = HandleEntry->KernelData;
+ Object = Entry->KernelData;
}
else
{
@@ -947,26 +983,15 @@
* Debugging code. Report attempts to lock deleted handles and
* locking type mismatches.
*/
-
- if ((HandleType & ~GDI_HANDLE_REUSE_MASK) == 0)
- {
- DPRINT1("Attempted to lock object 0x%x that is deleted!\n", hObj);
- KeRosDumpStackFrames(NULL, 20);
- }
- else
- {
- DPRINT1("Attempted to lock object 0x%x, type mismatch (0x%x : 0x%x)\n",
- hObj, HandleType & ~GDI_HANDLE_REUSE_MASK, ObjectType & ~GDI_HANDLE_REUSE_MASK);
-
- KeRosDumpStackFrames(NULL, 20);
- }
+ LockErrorDebugOutput(hObj, Entry, "GDIOBJ_ShareLockObj");
+
#ifdef GDI_DEBUG
DPRINT1("-> called from %s:%i\n", file, line);
#endif
}
/* Unlock the handle table entry. */
- (void)InterlockedExchangePointer(&HandleEntry->ProcessId, PrevProcId);
+ (void)InterlockedExchangePointer(&Entry->ProcessId, PrevProcId);
break;
}
@@ -1069,14 +1094,16 @@
/* we're locking an object that belongs to our process. First calculate
the new object type including the stock object flag and then try to
exchange it.*/
+ /* FIXME: On Windows the higher 16 bit of the type field don't always match
+ the type from the handle, it is probably a storage type
+ (type = pen, storage = brush) */
NewType = GDI_HANDLE_GET_TYPE(*hObj);
- NewType |= NewType >> 16;
- NewType |= (ULONG_PTR)(*hObj) & GDI_HANDLE_REUSE_MASK;
+ NewType |= GDI_HANDLE_GET_UPPER(*hObj) >> GDI_ENTRY_UPPER_SHIFT;
/* This is the type that the object should have right now, save it */
OldType = NewType;
- /* As the object should be a stock object, set it's flag, but only in the upper 16 bits */
- NewType |= GDI_HANDLE_STOCK_MASK;
+ /* As the object should be a stock object, set it's flag, but only in the lower 16 bits */
+ NewType |= GDI_ENTRY_STOCK_MASK;
/* Try to exchange the type field - but only if the old (previous type) matches! */
PrevType = InterlockedCompareExchange(&Entry->Type, NewType, OldType);
@@ -1198,7 +1225,7 @@
{
PETHREAD PrevThread;
- if((Entry->Type & ~GDI_HANDLE_REUSE_MASK) != 0 && Entry->KernelData != NULL)
+ if((Entry->Type & ~GDI_ENTRY_REUSE_MASK) != 0 && Entry->KernelData != NULL)
{
PGDIOBJHDR GdiHdr = GDIBdyToHdr(Entry->KernelData);
@@ -1334,7 +1361,7 @@
PETHREAD PrevThread;
PGDIOBJHDR GdiHdr;
- if((FromEntry->Type & ~GDI_HANDLE_REUSE_MASK) != 0 && FromEntry->KernelData != NULL)
+ if((FromEntry->Type & ~GDI_ENTRY_REUSE_MASK) != 0 && FromEntry->KernelData != NULL)
{
GdiHdr = GDIBdyToHdr(FromEntry->KernelData);
More information about the Ros-diffs
mailing list