[ros-dev] [ros-diffs] [jgardou] 48465: [WIN32K] - Rewrite NtGdiStretchDIBitsInternal : clearer, faster, stronger (+1 wine test)
Timo Kreuzer
timo.kreuzer at web.de
Fri Aug 6 17:03:57 UTC 2010
Hi,
Please take care about proper protection of the user mode buffer. The
current solution: probe and forget is not safe.
Possibilities are:
1) SEH protected copying of the buffer, pass the copy of the buffer to
lower level functions -> Easy to do, large overhead for large bitmaps.
2) SEH protected call to a lower level function, passing the user mode
buffer. -> Not possible if the lower level function is either allocating
any resources (unless also protected by SEH + finally) or can pass
execution to 3rd party provided code, like drivers.
3) Be sure to have SEH at the lowest level (DIB) -> Not possible as the
function might end up in a driver.
4) Use Mm to protect the buffer. Either with MmSecureVirtualMemory or
double mapping using MmProbeAndLockPages + MmGetSystemAddressForMdlSafe.
I think 4 is the way to go. While the overhead of remapping should be
relatively small compared to a full copy, we are still wasting large
ammounts of system address space.
MmSecureVirtualMemory might at first sound like a good solution, but
beware, it has some pitfalls. While it protects a memory range from
being freed, it doesn't protect it from being paged out. That wouldn't
be a problem, unless the memory is not backed by the page file, but let
say a network resource, which becomes unavailable after a page was paged
out. In this case we would get an in page error when trying to access
the page, leading to a kernel crash. So unless we can be sure that the
memory is backed by the page file, we need to additionally lock the
pages into memory to be safe. Final thing to note is that
MmSecureVirtualMemory is not implemented yet, but I hope with current
work on the VAD code, we'll soon get a present (hint).
Regards,
Timo
jgardou at svn.reactos.org wrote:
> Author: jgardou
> Date: Thu Aug 5 21:12:57 2010
> New Revision: 48465
>
> URL: http://svn.reactos.org/svn/reactos?rev=48465&view=rev
> Log:
> [WIN32K]
> - Rewrite NtGdiStretchDIBitsInternal : clearer, faster, stronger (+1 wine test)
>
> Modified:
> branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c
>
> Modified: branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c
> URL: http://svn.reactos.org/svn/reactos/branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c?rev=48465&r1=48464&r2=48465&view=diff
> ==============================================================================
> --- branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c [iso-8859-1] (original)
> +++ branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c [iso-8859-1] Thu Aug 5 21:12:57 2010
> @@ -290,7 +290,7 @@
>
> if(!(psurfSrc && psurfDst))
> {
> - DPRINT1("Error, could not lock surfaces.\n");
> + DPRINT1("Error, could not lock surfaces\n");
> goto cleanup;
> }
>
> @@ -663,15 +663,15 @@
> if(pbmci)
> {
> pbmci->bmciHeader.bcWidth = psurf->SurfObj.sizlBitmap.cx;
> - pbmci->bmciHeader.bcHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
> - -psurf->SurfObj.sizlBitmap.cy :
> + pbmci->bmciHeader.bcHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
> + -psurf->SurfObj.sizlBitmap.cy :
> psurf->SurfObj.sizlBitmap.cy;
> pbmci->bmciHeader.bcPlanes = 1;
> pbmci->bmciHeader.bcBitCount = BitsPerFormat(psurf->SurfObj.iBitmapFormat);
> }
> Info->bmiHeader.biWidth = psurf->SurfObj.sizlBitmap.cx;
> - Info->bmiHeader.biHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
> - -psurf->SurfObj.sizlBitmap.cy :
> + Info->bmiHeader.biHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
> + -psurf->SurfObj.sizlBitmap.cy :
> psurf->SurfObj.sizlBitmap.cy;;
> Info->bmiHeader.biPlanes = 1;
> Info->bmiHeader.biBitCount = BitsPerFormat(psurf->SurfObj.iBitmapFormat);
> @@ -711,15 +711,15 @@
> case 8:
> Info->bmiHeader.biClrUsed = 0;
>
> - /* If the bitmap if a DIB section and has the same format than what
> + /* If the bitmap if a DIB section and has the same format than what
> * we're asked, go ahead! */
> - if((psurf->hSecure) &&
> + if((psurf->hSecure) &&
> (BitsPerFormat(psurf->SurfObj.iBitmapFormat) == bpp))
> {
> if(Usage == DIB_RGB_COLORS)
> {
> unsigned int colors = min(psurf->ppal->NumColors, 1 << bpp);
> -
> +
> if(pbmci)
> {
> for(i=0; i < colors; i++)
> @@ -773,7 +773,7 @@
> rgbTriples[i].rgbtGreen = pDcPal->IndexedColors[i].peGreen;
> rgbTriples[i].rgbtBlue = pDcPal->IndexedColors[i].peBlue;
> }
> -
> +
> rgbQuads[i].rgbRed = pDcPal->IndexedColors[i].peRed;
> rgbQuads[i].rgbGreen = pDcPal->IndexedColors[i].peGreen;
> rgbQuads[i].rgbBlue = pDcPal->IndexedColors[i].peBlue;
> @@ -822,7 +822,7 @@
> {
> for(g = 0; g <= 5; g++)
> {
> - for(b = 0; b <= 5; b++)
> + for(b = 0; b <= 5; b++)
> {
> colorTriple->rgbtRed = (r * 0xff) / 5;
> colorTriple->rgbtGreen = (g * 0xff) / 5;
> @@ -935,7 +935,7 @@
> /* Restore them */
> Info->bmiHeader.biWidth = width;
> Info->bmiHeader.biHeight = height;
> -
> +
> if(!hBmpDest)
> {
> DPRINT1("Unable to create a DIB Section!\n");
> @@ -972,7 +972,7 @@
> _SEH2_TRY
> {
> RtlCopyMemory(Bits, pDIBits, DIB_GetDIBImageBytes (width, height, bpp));
> - }
> + }
> _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> {
> Status = _SEH2_GetExceptionCode();
> @@ -1021,95 +1021,104 @@
> UINT cjMaxBits,
> HANDLE hcmXform)
> {
> - HBITMAP hBitmap, hOldBitmap = NULL;
> - HDC hdcMem = NULL;
> + PDC pdc;
> + INT ret = 0;
> + LONG height;
> + LONG width;
> + WORD planes, bpp;
> + DWORD compr, size;
> + HBITMAP hBitmap;
> + BOOL fastpath = FALSE;
> NTSTATUS Status = STATUS_SUCCESS;
> - PVOID pvDIBits;
> - INT Ret = 0;
> +
>
> if (!Bits || !BitsInfo)
> - {
> - SetLastWin32Error(ERROR_INVALID_PARAMETER);
> return 0;
> - }
> -
> - /* Create a DIB Section, data will be probed there */
> - hBitmap = NtGdiCreateDIBSection(hDC,
> - NULL,
> - 0,
> - BitsInfo,
> - Usage,
> - 0,
> - 0,
> - 0,
> - &pvDIBits);
> -
> - if(!hBitmap)
> - {
> - DPRINT1("Failed to create a DIB.\n");
> - return 0;
> - }
> -
> - /* Set bits */
> - _SEH2_TRY
> - {
> - ProbeForRead(Bits, cjMaxBits, 1);
> - RtlCopyMemory(pvDIBits, Bits, DIB_GetDIBImageBytes(BitsInfo->bmiHeader.biWidth,
> - BitsInfo->bmiHeader.biHeight,
> - BitsInfo->bmiHeader.biBitCount));
> - }
> - _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> - {
> - Status = _SEH2_GetExceptionCode();
> - }
> - _SEH2_END
> -
> - if(!NT_SUCCESS(Status))
> - {
> - DPRINT1("Error : Could not read DIB bits\n");
> - SetLastNtError(Status);
> - goto cleanup;
> - }
> -
> - hdcMem = NtGdiCreateCompatibleDC(0);
> - if(!hdcMem)
> - {
> - DPRINT1("Failed to create a memory DC!");
> - goto cleanup;
> - }
> -
> - hOldBitmap = NtGdiSelectBitmap(hdcMem, hBitmap);
> - if(!hOldBitmap)
> - {
> - DPRINT1("Could not select the DIB into the memory DC\n");
> - goto cleanup;
> - }
> -
> - /* Do we want to stretch ? */
> - if((SrcWidth == DestWidth) && (SrcHeight == DestHeight))
> - {
> - Ret = NtGdiBitBlt(hDC, XDest, YDest, XDest + DestWidth, YDest + DestHeight,
> - hdcMem, XSrc, YSrc, ROP, 0, 0);
> - }
> - else
> - {
> - Ret = NtGdiStretchBlt(hDC, XDest, YDest, XDest + DestWidth, YDest + DestHeight,
> - hdcMem, XSrc, YSrc, XSrc + SrcWidth, YSrc + SrcHeight,
> - ROP, 0);
> - }
> -
> - if(Ret)
> - Ret = SrcHeight ;
> -
> +
> + if (!(pdc = DC_LockDc(hDC))) return 0;
> +
> +
> + _SEH2_TRY
> + {
> + ProbeForRead(BitsInfo, cjMaxInfo, 1);
> + ProbeForRead(Bits, cjMaxBits, 1);
> + if (DIB_GetBitmapInfo( &BitsInfo->bmiHeader, &width, &height, &planes, &bpp, &compr, &size ) == -1)
> + {
> + DPRINT1("Invalid bitmap\n");
> + Status = STATUS_INVALID_PARAMETER;
> + }
> + }
> + _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> + {
> + Status = _SEH2_GetExceptionCode();
> + }
> + _SEH2_END
> +
> + if(!NT_SUCCESS(Status))
> + {
> + DPRINT1("Error, failed to read the DIB bits\n");
> + goto cleanup;
> + }
> +
> + if (width < 0)
> + {
> + DPRINT1("Bitmap has a negative width\n");
> + return 0;
> + }
> +
> + hBitmap = NtGdiGetDCObject(hDC, OBJ_BITMAP);
> +
> + if (XDest == 0 && YDest == 0 && XSrc == 0 && XSrc == 0 &&
> + DestWidth == SrcWidth && DestHeight == SrcHeight &&
> + compr == BI_RGB &&
> + ROP == SRCCOPY)
> + {
> + BITMAP bmp;
> + if (IntGdiGetObject(hBitmap, sizeof(bmp), &bmp) == sizeof(bmp))
> + {
> + if (bmp.bmBitsPixel == bpp &&
> + bmp.bmWidth == SrcWidth &&
> + bmp.bmHeight == SrcHeight &&
> + bmp.bmPlanes == planes)
> + fastpath = TRUE;
> + }
> + }
> +
> + if (fastpath)
> + {
> + /* fast path */
> + DPRINT1("using fast path\n");
> + ret = IntSetDIBits( pdc, hBitmap, 0, height, Bits, BitsInfo, Usage);
> + }
> + else
> + {
> + /* slow path - need to use StretchBlt */
> + HBITMAP hOldBitmap;
> + HPALETTE hpal = NULL;
> + HDC hdcMem;
> + PVOID pvBits;
> +
> + hdcMem = NtGdiCreateCompatibleDC( hDC );
> + hBitmap = DIB_CreateDIBSection(pdc, BitsInfo, Usage, &pvBits, NULL, 0, 0);
> + RtlCopyMemory(pvBits, Bits, cjMaxBits);
> + hOldBitmap = NtGdiSelectBitmap( hdcMem, hBitmap );
> +
> + /* Origin for DIBitmap may be bottom left (positive biHeight) or top
> + left (negative biHeight) */
> + ret = NtGdiStretchBlt( hDC, XDest, YDest, DestWidth, DestHeight,
> + hdcMem, XSrc, abs(height) - SrcHeight - YSrc,
> + SrcWidth, SrcHeight, ROP, 0 );
> + if(hpal)
> + GdiSelectPalette(hdcMem, hpal, FALSE);
> + if(ret)
> + ret = SrcHeight;
> + NtGdiSelectBitmap( hdcMem, hOldBitmap );
> + NtGdiDeleteObjectApp( hdcMem );
> + GreDeleteObject( hBitmap );
> + }
> cleanup:
> - if(hdcMem)
> - {
> - if(hOldBitmap) NtGdiSelectBitmap(hdcMem, hOldBitmap);
> - NtGdiDeleteObjectApp(hdcMem);
> - }
> - GreDeleteObject(hBitmap);
> -
> - return Ret;
> + DC_UnlockDc(pdc);
> + return ret;
> }
>
>
> @@ -1462,7 +1471,7 @@
> hpal = (HPALETTE) 0xFFFFFFFF;
> }
> }
> - else
> + else
> {
> hpal = BuildDIBPalette(bmi);
> }
> @@ -1563,7 +1572,7 @@
> * Get the info from a bitmap header.
> * Return 0 for COREHEADER, 1 for INFOHEADER, -1 for error.
> */
> -int
> +int
> FASTCALL
> DIB_GetBitmapInfo( const BITMAPINFOHEADER *header, LONG *width,
> LONG *height, WORD *planes, WORD *bpp, DWORD *compr, DWORD *size )
> @@ -1575,7 +1584,7 @@
> *height = core->bcHeight;
> *planes = core->bcPlanes;
> *bpp = core->bcBitCount;
> - *compr = 0;
> + *compr = BI_RGB;
> *size = 0;
> return 0;
> }
> @@ -1692,10 +1701,10 @@
> ppalEntries[i].peBlue = 0;
> ppalEntries[i].peFlags = 0;
> }
> -
> +
> lpIndex++;
> }
> -
> +
> hpal = PALETTE_AllocPalette(PAL_INDEXED, nNumColors, (ULONG*)ppalEntries, 0, 0, 0);
>
> ExFreePoolWithTag(ppalEntries, TAG_COLORMAP);
>
>
>
>
More information about the Ros-dev
mailing list