[tkreuzer] 66608: [WIN32K] - revert an "improvement" in NtUserFindExistingCursorIcon - Remove boken asserts - Implement GreSetBitmapOwner and use it to set bitmap owner in IntSetCursorData - Fix cl...
8 Mar
2015
8 Mar
'15
1:44 p.m.
Author: tkreuzer
Date: Sun Mar 8 13:44:24 2015
New Revision: 66608
URL: http://svn.reactos.org/svn/reactos?rev=66608&view=rev
Log:
[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
Modified:
trunk/reactos/win32ss/gdi/ntgdi/bitmaps.c
trunk/reactos/win32ss/gdi/ntgdi/bitmaps.h
trunk/reactos/win32ss/user/ntuser/cursoricon.c
Modified: trunk/reactos/win32ss/gdi/ntgdi/bitmaps.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/gdi/ntgdi/bitmaps.…
==============================================================================
--- trunk/reactos/win32ss/gdi/ntgdi/bitmaps.c [iso-8859-1] (original)
+++ trunk/reactos/win32ss/gdi/ntgdi/bitmaps.c [iso-8859-1] Sun Mar 8 13:44:24 2015
@@ -10,6 +10,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
Modified: trunk/reactos/win32ss/gdi/ntgdi/bitmaps.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/gdi/ntgdi/bitmaps.…
==============================================================================
--- trunk/reactos/win32ss/gdi/ntgdi/bitmaps.h [iso-8859-1] (original)
+++ trunk/reactos/win32ss/gdi/ntgdi/bitmaps.h [iso-8859-1] Sun Mar 8 13:44:24 2015
@@ -2,6 +2,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
Modified: trunk/reactos/win32ss/user/ntuser/cursoricon.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/cursor…
==============================================================================
--- trunk/reactos/win32ss/user/ntuser/cursoricon.c [iso-8859-1] (original)
+++ trunk/reactos/win32ss/user/ntuser/cursoricon.c [iso-8859-1] Sun Mar 8 13:44:24 2015
@@ -107,8 +107,6 @@
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 @@
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 @@
/* 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 @@
_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 @@
{
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))
- {
- goto Cleanup;
+ if (!GreSetBitmapOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_PUBLIC))
+ {
+ 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))
- {
- goto Cleanup;
+ if (!GreSetBitmapOwner(pcursordata->hbmColor, GDI_OBJ_HMGR_PUBLIC))
+ {
+ ERR("Failed to set ownership of hbmColor %p.\n",
pcursordata->hbmColor);
+ GreSetBitmapOwner(pcursordata->hbmMask, GDI_OBJ_HMGR_POWNED);
+ return FALSE;
}
}
@@ -1156,19 +1160,27 @@
if (pcursordata->hbmAlpha)
{
/* Take ownership of the alpha bitmap */
- if (!GreSetObjectOwner(pcursordata->hbmAlpha, GDI_OBJ_HMGR_PUBLIC))
- {
- goto Cleanup;
- }
- }
-
- /* Free the old name */
+ if (!GreSetBitmapOwner(pcursordata->hbmAlpha, GDI_OBJ_HMGR_PUBLIC))
+ {
+ 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 (Must be NULL atm, but later we might allow this) */
+ NT_ASSERT(pcur->strName.Buffer == NULL);
if (pcur->strName.Buffer != NULL)
{
- ExFreePoolWithTag(pcur->strName.Buffer, TAG_STRING);
- pcur->strName.Buffer = NULL;
- pcur->strName.Length = 0;
- pcur->strName.MaximumLength = 0;
+ if (!IS_INTRESOURCE(pcur->strName.Buffer))
+ {
+ ExFreePoolWithTag(pcur->strName.Buffer, TAG_STRING);
+ }
+ RtlInitEmptyUnicodeString(&pcur->strName, NULL, 0);
}
/* Free the module atom */
@@ -1200,28 +1212,6 @@
}
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 @@
_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 @@
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);
@@ -1259,6 +1251,20 @@
{
ERR("Acon data already set!\n");
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
@@ -1280,6 +1286,13 @@
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 @@
if (hcurFrame == NULL)
{
ERR("Failed to create a cursor for frame %u\n", i);
- aspcur[i] = NULL;
goto Cleanup;
}
@@ -1299,8 +1311,13 @@
aspcur[i] = UserGetCurIconObject(hcurFrame);
NT_ASSERT(aspcur[i] != NULL);
- /* Mark this cursor as an acon frame */
- pcdFrame->CURSORF_flags |= CURSORF_ACONFRAME;
+ /* Check if the flags are valid */
+ 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 @@
ERR("Failed to set cursor data for frame %u\n", i);
goto Cleanup;
}
- }
-
- /* Free the old name */
+
+ /* Mark this cursor as an acon frame */
+ aspcur[i]->CURSORF_flags |= CURSORF_ACONFRAME;
+ }
+
+ /* Free the old name (Must be NULL atm.) */
+ NT_ASSERT(pacon->strName.Buffer == NULL);
if (pacon->strName.Buffer != NULL)
{
- ExFreePoolWithTag(pacon->strName.Buffer, TAG_STRING);
- pacon->strName.Buffer = NULL;
- pacon->strName.Length = 0;
- pacon->strName.MaximumLength = 0;
+ if (!IS_INTRESOURCE(pacon->strName.Buffer))
+ {
+ ExFreePoolWithTag(pacon->strName.Buffer, TAG_STRING);
+ }
+ RtlInitEmptyUnicodeString(&pacon->strName, NULL, 0);
}
/* Free the module atom */
@@ -1330,7 +1352,7 @@
{
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 @@
break;
/* Destroy this cursor */
- NT_VERIFY(UserDereferenceObject(aspcur[i]) == TRUE);
+ UserDereferenceObject(aspcur[i]);
NT_VERIFY(IntDestroyCurIconObject(aspcur[i]) == TRUE);
}
@@ -1472,6 +1494,9 @@
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 @@
goto Exit;
}
- /* Set the pointers */
- cursordata.aspcur = (CURSORDATA*)pvBuffer;
- cursordata.aicur = (DWORD*)&cursordata.aspcur[cursordata.cpcur];
- cursordata.ajifRate = (INT*)&cursordata.aicur[cursordata.cicur];
+ /* Calculate the kernel mode pointers */
+ aspcur = (CURSORDATA*)pvBuffer;
+ aicur = (DWORD*)&aspcur[cursordata.cpcur];
+ ajifRate = (INT*)&aicur[cursordata.cicur];
/* Probe and copy aspcur */
- ProbeForRead(pCursorData->aspcur, cursordata.cpcur * sizeof(CURSORDATA),
1);
- RtlCopyMemory(cursordata.aspcur,
- pCursorData->aspcur,
+ ProbeForRead(cursordata.aspcur, cursordata.cpcur * sizeof(CURSORDATA), 1);
+ RtlCopyMemory(aspcur,
+ cursordata.aspcur,
cursordata.cpcur * sizeof(CURSORDATA));
/* Probe and copy aicur */
- ProbeForRead(pCursorData->aicur, cursordata.cicur * sizeof(DWORD), 1);
- RtlCopyMemory(cursordata.aicur,
- pCursorData->aicur,
+ ProbeForRead(cursordata.aicur, cursordata.cicur * sizeof(DWORD), 1);
+ RtlCopyMemory(aicur,
+ cursordata.aicur,
cursordata.cicur * sizeof(DWORD));
/* Probe and copy ajifRate */
- ProbeForRead(pCursorData->ajifRate, cursordata.cicur * sizeof(INT), 1);
- RtlCopyMemory(cursordata.ajifRate,
- pCursorData->ajifRate,
+ ProbeForRead(cursordata.ajifRate, cursordata.cicur * sizeof(INT), 1);
+ RtlCopyMemory(ajifRate,
+ cursordata.ajifRate,
cursordata.cicur * sizeof(INT));
+
+ /* Set the new pointers */
+ cursordata.aspcur = aspcur;
+ cursordata.aicur = aicur;
+ cursordata.ajifRate = ajifRate;
}
else
{
@@ -1579,6 +1609,13 @@
ERR("Failed to copy pustrRsrc: status 0x%08lx\n", status);
goto Exit;
}
+ }
+
+ /* 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 */
3582
days inactive
3582
days old
0 comments
1 participants
participants (1)
-
tkreuzer＠svn.reactos.org