[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 i

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