[WIN32K]

- revert an "improvement" in NtUserFindExistingCursorIcon
- Remove boken asserts
- Implement GreSetBitmapOwner and use it to set bitmap owner in IntSetCursorData
- Fix cleanup after failure in setting bitmap owner
- Fix string cleanup (don't free INTRESOURCE)
- Validate frame indices to be within range
- Make sure frame indices and JIR reates are copied
- A few other fixes/improvements

svn path=/trunk/; revision=66608

reactos/win32ss/gdi/ntgdi/bitmaps.c

@@ -11,6 +11,37 @@
 #define NDEBUG
 #include <debug.h>
 
+BOOL
+NTAPI
+GreSetBitmapOwner(
+    _In_ HBITMAP hbmp,
+    _In_ ULONG ulOwner)
+{
+    /* Check if we have the correct object type */
+    if (GDI_HANDLE_GET_TYPE(hbmp) != GDILoObjType_LO_BITMAP_TYPE)
+    {
+        DPRINT1("Incorrect type for hbmp: %p\n", hbmp);
+        return FALSE;
+    }
+
+    /// FIXME: this is a hack and doesn't handle a race condition properly.
+    /// It needs to be done in GDIOBJ_vSetObjectOwner atomically.
+
+    /* Check if we set public or none */
+    if ((ulOwner == GDI_OBJ_HMGR_PUBLIC) ||
+        (ulOwner == GDI_OBJ_HMGR_NONE))
+    {
+        /* Only allow this for owned objects */
+        if (GreGetObjectOwner(hbmp) != GDI_OBJ_HMGR_POWNED)
+        {
+            DPRINT1("Cannot change owner for non-powned hbmp\n");
+            return FALSE;
+        }
+    }
+
+    return GreSetObjectOwner(hbmp, ulOwner);
+}
+
 static
 int
 NTAPI

reactos/win32ss/gdi/ntgdi/bitmaps.h

@@ -3,6 +3,12 @@
 INT     APIENTRY  BITMAP_GetObject(SURFACE * bmp, INT count, LPVOID buffer);
 HBITMAP FASTCALL BITMAP_CopyBitmap (HBITMAP  hBitmap);
 
+BOOL
+NTAPI
+GreSetBitmapOwner(
+    _In_ HBITMAP hbmp,
+    _In_ ULONG ulOwner);
+
 HBITMAP
 NTAPI
 GreCreateBitmap(

reactos/win32ss/user/ntuser/cursoricon.c

@@ -107,8 +107,6 @@ IntRemoveCursorFromList(
     NT_ASSERT((pcur->CURSORF_flags & (CURSORF_GLOBAL|CURSORF_LRSHARED)) != 0);
     NT_ASSERT((pcur->CURSORF_flags & CURSORF_LINKED) != 0);
 
-    NT_ASSERT((pcur->CURSORF_flags & CURSORF_GLOBAL) == 0);
-
     /* Get the right list head */
     ppcurHead = (pcur->CURSORF_flags & CURSORF_GLOBAL) ?
         &gcurFirst : &ppi->pCursorCache;
@@ -360,7 +358,7 @@ FreeCurIconObject(
 
         for (i = 0; i < AniCurIcon->cpcur; i++)
         {
-            NT_VERIFY(UserDereferenceObject(AniCurIcon->aspcur[i]) == TRUE);
+            UserDereferenceObject(AniCurIcon->aspcur[i]);
             NT_VERIFY(IntDestroyCurIconObject(AniCurIcon->aspcur[i]) == TRUE);
         }
         ExFreePoolWithTag(AniCurIcon->aspcur, USERTAG_CURSOR);
@@ -904,11 +902,16 @@ NtUserFindExistingCursorIcon(
         /* See if module names match */
         if (atomModName == CurIcon->atomModName)
         {
-            /* Check if this is an INTRESOURCE */
+            /* They do. Now see if this is the same resource */
+            if (IS_INTRESOURCE(CurIcon->strName.Buffer) != IS_INTRESOURCE(ustrRsrcSafe.Buffer))
+            {
+                /* One is an INT resource and the other is not -> no match */
+                CurIcon = CurIcon->pcurNext;
+                continue;
+            }
+
             if (IS_INTRESOURCE(CurIcon->strName.Buffer))
             {
-                /* Compare if it matches the one we are looking for. This also
-                   handles the case, where ustrRsrcSafe is not an INTRESOURCE */
                 if (CurIcon->strName.Buffer == ustrRsrcSafe.Buffer)
                 {
                     /* INT resources match */
@@ -1111,8 +1114,6 @@ IntSetCursorData(
     _In_ ATOM atomModName,
     _In_ const CURSORDATA* pcursordata)
 {
-    NT_ASSERT((pcur->CURSORF_flags & CURSORF_ACON) == 0);
-
     /* Check if the CURSORF_ACON is also set in the cursor data */
     if (pcursordata->CURSORF_flags & CURSORF_ACON)
     {
@@ -1133,22 +1134,25 @@ IntSetCursorData(
     {
         ERR("NtUserSetCursorIconData was got no hbmMask.\n");
         EngSetLastError(ERROR_INVALID_PARAMETER);
-        goto Cleanup;
+        return FALSE;
     }
 
     /* Take ownership of the mask bitmap */
-    if (!GreSetObjectOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_PUBLIC))
+    if (!GreSetBitmapOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_PUBLIC))
     {
-        goto Cleanup;
+        ERR("Failed to set ownership of hbmMask %p.\n", pcursordata->hbmMask);
+        return FALSE;
     }
 
     /* Check if we have a color bitmap */
     if (pcursordata->hbmColor)
     {
         /* Take ownership of the color bitmap */
-        if (!GreSetObjectOwner(pcursordata->hbmColor, GDI_OBJ_HMGR_PUBLIC))
+        if (!GreSetBitmapOwner(pcursordata->hbmColor, GDI_OBJ_HMGR_PUBLIC))
         {
-            goto Cleanup;
+            ERR("Failed to set ownership of hbmColor %p.\n", pcursordata->hbmColor);
+            GreSetBitmapOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_POWNED);
+            return FALSE;
         }
     }
 
@@ -1156,19 +1160,27 @@ IntSetCursorData(
     if (pcursordata->hbmAlpha)
     {
         /* Take ownership of the alpha bitmap */
-        if (!GreSetObjectOwner(pcursordata->hbmAlpha, GDI_OBJ_HMGR_PUBLIC))
+        if (!GreSetBitmapOwner(pcursordata->hbmAlpha, GDI_OBJ_HMGR_PUBLIC))
         {
-            goto Cleanup;
+            ERR("Failed to set ownership of hbmAlpha %p.\n", pcursordata->hbmAlpha);
+            GreSetBitmapOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_POWNED);
+            if (pcursordata->hbmColor)
+            {
+                GreSetBitmapOwner(pcursordata->hbmColor, GDI_OBJ_HMGR_POWNED);
+            }
+            return FALSE;
         }
     }
 
-    /* Free the old name */
+    /* Free the old name (Must be NULL atm, but later we might allow this) */
+    NT_ASSERT(pcur->strName.Buffer == NULL);
     if (pcur->strName.Buffer != NULL)
+    {
+        if (!IS_INTRESOURCE(pcur->strName.Buffer))
         {
             ExFreePoolWithTag(pcur->strName.Buffer, TAG_STRING);
-        pcur->strName.Buffer = NULL;
+        }
-        pcur->strName.Length = 0;
+        RtlInitEmptyUnicodeString(&pcur->strName, NULL, 0);
-        pcur->strName.MaximumLength = 0;
     }
 
     /* Free the module atom */
@@ -1200,28 +1212,6 @@ IntSetCursorData(
     }
 
     return TRUE;
-
-Cleanup:
-
-    if (pcursordata->hbmMask != NULL)
-    {
-        GreSetObjectOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_POWNED);
-        GreDeleteObject(pcursordata->hbmMask);
-    }
-
-    if (pcursordata->hbmColor != NULL)
-    {
-        GreSetObjectOwner(pcursordata->hbmColor, GDI_OBJ_HMGR_POWNED);
-        GreDeleteObject(pcursordata->hbmColor);
-    }
-
-    if (pcursordata->hbmAlpha != NULL)
-    {
-        GreSetObjectOwner(pcursordata->hbmAlpha, GDI_OBJ_HMGR_POWNED);
-        GreDeleteObject(pcursordata->hbmAlpha);
-    }
-
-    return FALSE;
 }
 
 static
@@ -1230,7 +1220,7 @@ IntSetAconData(
     _Inout_ PACON pacon,
     _In_opt_ PUNICODE_STRING pustrName,
     _In_ ATOM atomModName,
-    _In_ PCURSORDATA pcursordata)
+    _In_ const CURSORDATA *pcursordata)
 {
     PCURICON_OBJECT *aspcur;
     DWORD *aicur;
@@ -1240,9 +1230,11 @@ IntSetAconData(
     UINT cjSize, i;
 
     NT_ASSERT((pacon->CURSORF_flags & CURSORF_ACON) != 0);
+    NT_ASSERT((pacon->CURSORF_flags & CURSORF_ACONFRAME) == 0);
     NT_ASSERT((ULONG_PTR)pcursordata->aspcur > MmUserProbeAddress);
     NT_ASSERT((ULONG_PTR)pcursordata->aicur > MmUserProbeAddress);
     NT_ASSERT((ULONG_PTR)pcursordata->ajifRate > MmUserProbeAddress);
+    NT_ASSERT((pcursordata->CURSORF_flags & ~CURSORF_USER_MASK) == 0);
     NT_ASSERT(pcursordata->cpcur > 0);
     NT_ASSERT(pcursordata->cicur > 0);
 
@@ -1261,6 +1253,20 @@ IntSetAconData(
         return FALSE;
     }
 
+    /* Loop all frames indexes */
+    for (i = 0; i < pcursordata->cicur; i++)
+    {
+        /* Check if the index is within the range of the frames */
+        if (pcursordata->aicur[i] >= pcursordata->cpcur)
+        {
+            ERR("aicur[%lu] is out or range. Got %lu, cpcur = %u\n",
+                i, pcursordata->aicur[i], pcursordata->cpcur);
+            return FALSE;
+        }
+
+        /* FIXME: check the JIF rates? */
+    }
+
     /* Calculate size: one cursor object for each frame, and a frame
        index and jiffies for each "step" */
     cjSize = (pcursordata->cpcur * sizeof(CURICON_OBJECT*)) +
@@ -1280,6 +1286,13 @@ IntSetAconData(
     aicur = (DWORD*)&aspcur[pcursordata->cpcur];
     ajifRate = (INT*)&aicur[pcursordata->cicur];
 
+    /* Copy the values */
+    RtlCopyMemory(aicur, pcursordata->aicur, pcursordata->cicur * sizeof(DWORD));
+    RtlCopyMemory(ajifRate, pcursordata->ajifRate, pcursordata->cicur * sizeof(INT));
+
+    /* Zero out the array, so we can handle cleanup */
+    RtlZeroMemory(aspcur, pcursordata->cpcur * sizeof(PCURICON_OBJECT));
+
     /* Get a pointer to the cursor data for each frame */
     pcdFrame = pcursordata->aspcur;
 
@@ -1291,7 +1304,6 @@ IntSetAconData(
         if (hcurFrame == NULL)
         {
             ERR("Failed to create a cursor for frame %u\n", i);
-            aspcur[i] = NULL;
             goto Cleanup;
         }
 
@@ -1299,8 +1311,13 @@ IntSetAconData(
         aspcur[i] = UserGetCurIconObject(hcurFrame);
         NT_ASSERT(aspcur[i] != NULL);
 
-        /* Mark this cursor as an acon frame */
+        /* Check if the flags are valid */
-        pcdFrame->CURSORF_flags |= CURSORF_ACONFRAME;
+        if (pcdFrame->CURSORF_flags & ~(CURSORF_USER_MASK|CURSORF_ACONFRAME))
+        {
+            ERR("Invalid flags for acon frame %u: 0x%lx\n",
+                i, pcdFrame->CURSORF_flags);
+            goto Cleanup;
+        }
 
         /* Set the cursor data for this frame */
         if (!IntSetCursorData(aspcur[i], NULL, 0, &pcdFrame[i]))
@@ -1308,15 +1325,20 @@ IntSetAconData(
             ERR("Failed to set cursor data for frame %u\n", i);
             goto Cleanup;
         }
+
+        /* Mark this cursor as an acon frame */
+        aspcur[i]->CURSORF_flags |= CURSORF_ACONFRAME;
     }
 
-    /* Free the old name */
+    /* Free the old name (Must be NULL atm.) */
+    NT_ASSERT(pacon->strName.Buffer == NULL);
     if (pacon->strName.Buffer != NULL)
+    {
+        if (!IS_INTRESOURCE(pacon->strName.Buffer))
         {
             ExFreePoolWithTag(pacon->strName.Buffer, TAG_STRING);
-        pacon->strName.Buffer = NULL;
+        }
-        pacon->strName.Length = 0;
+        RtlInitEmptyUnicodeString(&pacon->strName, NULL, 0);
-        pacon->strName.MaximumLength = 0;
     }
 
     /* Free the module atom */
@@ -1330,7 +1352,7 @@ IntSetAconData(
     {
         for (i = 0; i < pacon->cpcur; i++)
         {
-            NT_VERIFY(UserDereferenceObject(pacon->aspcur[i]) == TRUE);
+            UserDereferenceObject(pacon->aspcur[i]);
             NT_VERIFY(IntDestroyCurIconObject(pacon->aspcur[i]) == TRUE);
         }
         ExFreePoolWithTag(pacon->aspcur, USERTAG_CURSOR);
@@ -1362,7 +1384,7 @@ Cleanup:
             break;
 
         /* Destroy this cursor */
-        NT_VERIFY(UserDereferenceObject(aspcur[i]) == TRUE);
+        UserDereferenceObject(aspcur[i]);
         NT_VERIFY(IntDestroyCurIconObject(aspcur[i]) == TRUE);
     }
 
@@ -1472,6 +1494,9 @@ NtUserSetCursorIconData(
     CURSORDATA cursordata;
     UNICODE_STRING ustrModule, ustrRsrc;
     _SEH2_VOLATILE PVOID pvBuffer;
+    CURSORDATA* aspcur;
+    DWORD* aicur;
+    PINT ajifRate;
     UINT cjSize;
     NTSTATUS status;
     BOOL bResult;
@@ -1518,28 +1543,33 @@ NtUserSetCursorIconData(
                 goto Exit;
             }
 
-            /* Set the pointers */
+            /* Calculate the kernel mode pointers */
-            cursordata.aspcur = (CURSORDATA*)pvBuffer;
+            aspcur = (CURSORDATA*)pvBuffer;
-            cursordata.aicur = (DWORD*)&cursordata.aspcur[cursordata.cpcur];
+            aicur = (DWORD*)&aspcur[cursordata.cpcur];
-            cursordata.ajifRate = (INT*)&cursordata.aicur[cursordata.cicur];
+            ajifRate = (INT*)&aicur[cursordata.cicur];
 
             /* Probe and copy aspcur */
-            ProbeForRead(pCursorData->aspcur, cursordata.cpcur * sizeof(CURSORDATA), 1);
+            ProbeForRead(cursordata.aspcur, cursordata.cpcur * sizeof(CURSORDATA), 1);
-            RtlCopyMemory(cursordata.aspcur,
+            RtlCopyMemory(aspcur,
-                          pCursorData->aspcur,
+                          cursordata.aspcur,
                           cursordata.cpcur * sizeof(CURSORDATA));
 
             /* Probe and copy aicur */
-            ProbeForRead(pCursorData->aicur, cursordata.cicur * sizeof(DWORD), 1);
+            ProbeForRead(cursordata.aicur, cursordata.cicur * sizeof(DWORD), 1);
-            RtlCopyMemory(cursordata.aicur,
+            RtlCopyMemory(aicur,
-                          pCursorData->aicur,
+                          cursordata.aicur,
                           cursordata.cicur * sizeof(DWORD));
 
             /* Probe and copy ajifRate */
-            ProbeForRead(pCursorData->ajifRate, cursordata.cicur * sizeof(INT), 1);
+            ProbeForRead(cursordata.ajifRate, cursordata.cicur * sizeof(INT), 1);
-            RtlCopyMemory(cursordata.ajifRate,
+            RtlCopyMemory(ajifRate,
-                          pCursorData->ajifRate,
+                          cursordata.ajifRate,
                           cursordata.cicur * sizeof(INT));
+
+            /* Set the new pointers */
+            cursordata.aspcur = aspcur;
+            cursordata.aicur = aicur;
+            cursordata.ajifRate = ajifRate;
         }
         else
         {
@@ -1581,6 +1611,13 @@ NtUserSetCursorIconData(
         }
     }
 
+    /* Make sure the caller doesn't give us invalid flags */
+    if (cursordata.CURSORF_flags & ~CURSORF_USER_MASK)
+    {
+        ERR("Invalid cursor flags: 0x%08lx\n", cursordata.CURSORF_flags);
+        goto Exit;
+    }
+
     /* Acquire the global user lock */
     UserEnterExclusive();