Re: [ros-dev] [ros-diffs] [tkreuzer] 70464: [WIN32K] Properly check the bitmap size in IntSetDIBits, taking into account that it might be set to 0, then we have to calculate it ourselves. Fixes a crash when invalid data is ...

[Thomas Faber]

Timo, this is causing testbot to freak out since it now crashes during
an apitest. Could you look into it?
Thanks.

On 2015-12-29 21:28, tkreuzer(a)svn.reactos.org wrote:
>   Author: tkreuzer
>  Date: Tue Dec 29 20:28:23 2015
>  New Revision: 70464
>  
>  URL: http://svn.reactos.org/svn/reactos?rev=70464&view=rev
>  Log:
>  [WIN32K]
>  Properly check the bitmap size in IntSetDIBits, taking into account that it might be set
> to 0, then we have to calculate it ourselves. Fixes a crash when invalid data is passed to
> NtGdiCreateDIBitmapInternal.
>  [GDI32]
>  Convert COREINFOHEADER to BITMAPINFOHEADER before passing it to
> NtGdiCreateDIBitmapInternal, which doesn't support it.
>  CORE-10583 #resolve
>  
>  Modified:
>      trunk/reactos/win32ss/gdi/gdi32/objects/bitmap.c
>      trunk/reactos/win32ss/gdi/ntgdi/dibobj.c
>  
>  Modified: trunk/reactos/win32ss/gdi/gdi32/objects/bitmap.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/gdi/gdi32/objects/…
>  ==============================================================================
>  --- trunk/reactos/win32ss/gdi/gdi32/objects/bitmap.c	[iso-8859-1] (original)
>  +++ trunk/reactos/win32ss/gdi/gdi32/objects/bitmap.c	[iso-8859-1] Tue Dec 29 20:28:23
> 2015
>  @@ -408,23 +408,30 @@
>   //  PDC_ATTR pDc_Attr;
>       UINT InfoSize = 0;
>       UINT cjBmpScanSize = 0;
>  -    HBITMAP hBmp;
>  +    HBITMAP hBmp = NULL;
>       NTSTATUS Status = STATUS_SUCCESS;
>  +    PBITMAPINFO pbmiConverted;
>  +    UINT cjInfoSize;
>  +
>  +    /* Convert the BITMAPINFO if it is a COREINFO */
>  +    pbmiConverted = ConvertBitmapInfo(Data, ColorUse, &cjInfoSize, FALSE);
>   
>       /* Check for CBM_CREATDIB */
>       if (Init & CBM_CREATDIB)
>       {
>           /* CBM_CREATDIB needs Data. */
>  -        if (!Data)
>  -        {
>  -            return 0;
>  +        if (pbmiConverted == NULL)
>  +        {
>  +            DPRINT1("CBM_CREATDIB needs a BITMAINFO!\n");
>  +            goto Exit;
>           }
>   
>           /* It only works with PAL or RGB */
>           if (ColorUse > DIB_PAL_COLORS)
>           {
>  +            DPRINT1("Invalid ColorUse: %lu\n", ColorUse);
>               GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -            return 0;
>  +            goto Exit;
>           }
>   
>           /* Use the header from the data */
>  @@ -434,38 +441,48 @@
>       /* Header is required */
>       if (!Header)
>       {
>  +        DPRINT1("Header is NULL\n");
>           GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -        return 0;
>  +        goto Exit;
>       }
>   
>       /* Get the bitmap format and dimensions */
>       if (DIB_GetBitmapInfo(Header, &width, &height, &planes, &bpp,
> &compr, &dibsize) == -1)
>       {
>  +        DPRINT1("DIB_GetBitmapInfo failed!\n");
>           GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -        return NULL;
>  +        goto Exit;
>       }
>   
>       /* Check if the Compr is incompatible */
>       if ((compr == BI_JPEG) || (compr == BI_PNG) || (compr == BI_BITFIELDS))
>  -        return 0;
>  +    {
>  +        DPRINT1("invalid compr: %lu!\n", compr);
>  +        goto Exit;
>  +    }
>   
>       /* Only DIB_RGB_COLORS (0), DIB_PAL_COLORS (1) and 2 are valid. */
>       if (ColorUse > DIB_PAL_COLORS + 1)
>       {
>  +        DPRINT1("invalid compr: %lu!\n", compr);
>           GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -        return 0;
>  +        goto Exit;
>       }
>   
>       /* If some Bits are given, only DIB_PAL_COLORS and DIB_RGB_COLORS are valid */
>       if (Bits && (ColorUse > DIB_PAL_COLORS))
>       {
>  +        DPRINT1("Invalid ColorUse: %lu\n", ColorUse);
>           GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -        return 0;
>  +        goto Exit;
>       }
>   
>       /* Negative width is not allowed */
>       if (width < 0)
>  -        return 0;
>  +    {
>  +        DPRINT1("Negative width: %li\n", width);
>  +        goto Exit;
>  +    }
>   
>       /* Top-down DIBs have a negative height. */
>       height = abs(height);
>  @@ -473,13 +490,13 @@
>   // For Icm support.
>   // GdiGetHandleUserData(hdc, GDI_OBJECT_TYPE_DC, (PVOID)&pDc_Attr))
>   
>  -    if (Data)
>  +    if (pbmiConverted)
>       {
>           _SEH2_TRY
>           {
>  -            cjBmpScanSize = GdiGetBitmapBitsSize((BITMAPINFO *) Data);
>  -            CalculateColorTableSize(&Data->bmiHeader, &ColorUse,
> &InfoSize);
>  -            InfoSize += Data->bmiHeader.biSize;
>  +            cjBmpScanSize = GdiGetBitmapBitsSize(pbmiConverted);
>  +            CalculateColorTableSize(&pbmiConverted->bmiHeader, &ColorUse,
> &InfoSize);
>  +            InfoSize += pbmiConverted->bmiHeader.biSize;
>           }
>           _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>           {
>  @@ -490,8 +507,9 @@
>   
>       if (!NT_SUCCESS(Status))
>       {
>  +        DPRINT1("Got an exception!\n");
>           GdiSetLastError(ERROR_INVALID_PARAMETER);
>  -        return NULL;
>  +        goto Exit;
>       }
>   
>       DPRINT("pBMI %p, Size bpp %u, dibsize %d, Conv %u, BSS %u\n", Data, bpp,
> dibsize, InfoSize,
>  @@ -501,9 +519,18 @@
>           hBmp = GetStockObject(DEFAULT_BITMAP);
>       else
>       {
>  -        hBmp = NtGdiCreateDIBitmapInternal(hDC, width, height, Init, (LPBYTE) Bits,
>  -            (LPBITMAPINFO) Data, ColorUse, InfoSize, cjBmpScanSize, 0, 0);
>  -    }
>  +        hBmp = NtGdiCreateDIBitmapInternal(hDC, width, height, Init, (LPBYTE)Bits,
>  +            (LPBITMAPINFO)pbmiConverted, ColorUse, InfoSize, cjBmpScanSize, 0, 0);
>  +    }
>  +
>  +Exit:
>  +
>  +    /* Cleanup converted BITMAPINFO */
>  +    if ((pbmiConverted != NULL) && (pbmiConverted != Data))
>  +    {
>  +        RtlFreeHeap(RtlGetProcessHeap(), 0, pbmiConverted);
>  +    }
>  +
>       return hBmp;
>   }
>   
>  
>  Modified: trunk/reactos/win32ss/gdi/ntgdi/dibobj.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/gdi/ntgdi/dibobj.c…
>  ==============================================================================
>  --- trunk/reactos/win32ss/gdi/ntgdi/dibobj.c	[iso-8859-1] (original)
>  +++ trunk/reactos/win32ss/gdi/ntgdi/dibobj.c	[iso-8859-1] Tue Dec 29 20:28:23 2015
>  @@ -259,11 +259,37 @@
>       POINTL		ptSrc;
>       EXLATEOBJ	exlo;
>       PPALETTE    ppalDIB = 0;
>  +    ULONG cjSizeImage;
>   
>       if (!bmi) return 0;
>   
>  -    if (bmi->bmiHeader.biSizeImage > cjMaxBits)
>  -    {
>  +    /* Check if the header provided an image size */
>  +    if (bmi->bmiHeader.biSizeImage != 0)
>  +    {
>  +        /* Use the given size */
>  +        cjSizeImage = bmi->bmiHeader.biSizeImage;
>  +    }
>  +    /* Otherwise check for uncompressed formats */
>  +    else if ((bmi->bmiHeader.biCompression == BI_RGB) ||
>  +             (bmi->bmiHeader.biCompression == BI_BITFIELDS))
>  +    {
>  +        /* Calculate the image size */
>  +        cjSizeImage = DIB_GetDIBImageBytes(bmi->bmiHeader.biWidth,
>  +                                           ScanLines,
>  +                                           bmi->bmiHeader.biBitCount);
>  +    }
>  +    else
>  +    {
>  +        /* Compressed format without a size. This is invalid. */
>  +        DPRINT1("Compressed format without a size!");
>  +        return 0;
>  +    }
>  +
>  +    /* Check if the size that we have is ok */
>  +    if (cjSizeImage > cjMaxBits)
>  +    {
>  +        DPRINT1("Size too large! cjSizeImage = %lu, cjMaxBits = %lu\n",
>  +                cjSizeImage, cjMaxBits);
>           return 0;
>       }
>   
>  @@ -273,7 +299,7 @@
>                                        BitmapFormat(bmi->bmiHeader.biBitCount,
>                                                     bmi->bmiHeader.biCompression),
>                                        bmi->bmiHeader.biHeight < 0 ? BMF_TOPDOWN :
> 0,
>  -                                     bmi->bmiHeader.biSizeImage,
>  +                                     cjSizeImage,
>                                        (PVOID)Bits,
>                                        0);
>       if (!SourceBitmap)
>  @@ -314,6 +340,8 @@
>       rcDst.right = psurfDst->SurfObj.sizlBitmap.cx;
>       ptSrc.x = 0;
>       ptSrc.y = 0;
>  +
>  +    NT_ASSERT(psurfSrc->SurfObj.cjBits <= cjMaxBits);
>   
>       result = IntEngCopyBits(&psurfDst->SurfObj,
>                               &psurfSrc->SurfObj,
>  @@ -1366,7 +1394,11 @@
>               /* Undocumented flag which creates a DDB of the format specified by the
> bitmap info. */
>               handle = IntCreateCompatibleBitmap(Dc, width, height, planes, bpp);
>               if (!handle)
>  +            {
>  +                DPRINT1("IntCreateCompatibleBitmap() failed!\n");
>                   return NULL;
>  +            }
>  +
>               /* The palette must also match the given data */
>               Surface = SURFACE_ShareLockSurface(handle);
>               ASSERT(Surface);
>  @@ -1430,6 +1462,7 @@
>           safeBits = ExAllocatePoolWithTag(PagedPool, cjMaxBits, TAG_DIB);
>           if(!safeBits)
>           {
>  +            DPRINT1("Failed to allocate %lu bytes\n", cjMaxBits);
>               EngSetLastError(ERROR_NOT_ENOUGH_MEMORY);
>               return NULL;
>           }
>  @@ -1452,6 +1485,7 @@
>   
>       if(!NT_SUCCESS(Status))
>       {
>  +        DPRINT1("Got an exception! pjInit = %p\n", pjInit);
>           SetLastNtError(Status);
>           goto cleanup;
>       }
>  @@ -1498,6 +1532,7 @@
>           hdcDest = NtGdiCreateCompatibleDC(0);
>           if(!hdcDest)
>           {
>  +            DPRINT1("NtGdiCreateCompatibleDC failed\n");
>               return NULL;
>           }
>       }
>  @@ -1509,6 +1544,7 @@
>       Dc = DC_LockDc(hdcDest);
>       if (!Dc)
>       {
>  +        DPRINT1("Failed to lock hdcDest %p\n", hdcDest);
>           EngSetLastError(ERROR_INVALID_HANDLE);
>           return NULL;
>       }
>  
>