From e7aacb51636b351eeee23f2fed45d24f6af3f87e Mon Sep 17 00:00:00 2001 From: Timo Kreuzer Date: Sun, 8 Mar 2015 13:44:24 +0000 Subject: [PATCH] [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 | 31 +++++ reactos/win32ss/gdi/ntgdi/bitmaps.h | 6 + reactos/win32ss/user/ntuser/cursoricon.c | 169 ++++++++++++++--------- 3 files changed, 140 insertions(+), 66 deletions(-) diff --git a/reactos/win32ss/gdi/ntgdi/bitmaps.c b/reactos/win32ss/gdi/ntgdi/bitmaps.c index 42dc6030b1b..3ef4ce90e6a 100644 --- a/reactos/win32ss/gdi/ntgdi/bitmaps.c +++ b/reactos/win32ss/gdi/ntgdi/bitmaps.c @@ -11,6 +11,37 @@ #define NDEBUG #include +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 diff --git a/reactos/win32ss/gdi/ntgdi/bitmaps.h b/reactos/win32ss/gdi/ntgdi/bitmaps.h index b8231765542..96c2611d659 100644 --- a/reactos/win32ss/gdi/ntgdi/bitmaps.h +++ b/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( diff --git a/reactos/win32ss/user/ntuser/cursoricon.c b/reactos/win32ss/user/ntuser/cursoricon.c index 2789507358c..f8db31f9272 100644 --- a/reactos/win32ss/user/ntuser/cursoricon.c +++ b/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) { - 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 @@ 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 */ - 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 @@ 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) { - 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 @@ 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 */ - 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 { @@ -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();