From: Indrek Altpere <efbiaiinzinz@hotmail.com>
Subject: winemenubuilder: fix crash caused by invalid icon entries and avoid future crashes by ignoring unhandled png entries (resend)
Message-Id: <DUB404-EAS120A9EAD03248E9E8C8228FCFDA0@phx.gbl>
Date: Thu, 28 Aug 2014 09:30:34 +0300

Forgot to link to the bug before, also added more detailed description.

Fixes https://bugs.winehq.org/show_bug.cgi?id=19241

For the InnoSetup 5 crash (and likely other similar reported crashes), the
issue seems to be GRPICONDIRENTRY with invalid information.
The dwBytesInRes has a value that exceeds the Size value in
IMAGE_RESOURCE_DATA_ENTRY, causing out-of-bounds memcpy and thus the crash.
dwBytesRes value 0x40028, as mentioned by Focht and existing in the
executable, seems to be the size of unpacked bitmap data (256x256x4 + 40
byte header) and not the actual size of compressed PNG bytes.
Added check+clipping against the out-of-bounds read, which fixes the
particular crash.

As per MSDN blog, icon resources can contain raw PNG information instead of
regular BITMAPINFO, but due to weird decisions, only way to differentiate
between them is to check if the resource starts with PNG header bytes.
http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
Made the winemenubuilder ignore such entries for now (with fixme notices),
since using PNG raw bytes as BITMAPINFO data is definitely invalid and is
due to cause other issues/crashes when the best matching size icon happens
to be PNG. A new bug should be opened for the missing PNG support.

Regards,
Indrek

diff --git a/programs/winemenubuilder/winemenubuilder.c b/programs/winemenubuilder/winemenubuilder.c
index 4675369..eac4cd6 100644
--- a/programs/winemenubuilder/winemenubuilder.c
+++ b/programs/winemenubuilder/winemenubuilder.c
@@ -599,6 +599,7 @@ static int populate_module_icons(HMODULE hModule, GRPICONDIR *grpIconDir, ICONDI
 {
     int i;
     int validEntries = 0;
+    static const BYTE png_header[] = { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
 
     for (i = 0; i < grpIconDir->idCount; i++)
     {
@@ -610,19 +611,32 @@ static int populate_module_icons(HMODULE hModule, GRPICONDIR *grpIconDir, ICONDI
             if ((hResData = LoadResource(hModule, hResInfo)))
             {
                 BITMAPINFO *pIcon;
+                DWORD availableBytes = ((IMAGE_RESOURCE_DATA_ENTRY *)hResInfo)->Size;
+                DWORD iconBytes = grpIconDir->idEntries[i].dwBytesInRes;
+                if (iconBytes > availableBytes)
+                {
+                    WINE_ERR("Broken GRPICONENTRY tries to use %d bytes when only %d available, clipping\n", iconBytes, availableBytes);
+                    iconBytes = availableBytes;
+                }
                 if ((pIcon = LockResource(hResData)))
                 {
+                    BOOL isPNG = iconBytes >= sizeof(png_header) && memcmp(pIcon, png_header, sizeof(png_header)) == 0;
+                    if (isPNG)
+                    {
+                        WINE_FIXME("PNG icon not supported\n");
+                        continue;
+                    }
                     iconDirEntries[validEntries].bWidth = grpIconDir->idEntries[i].bWidth;
                     iconDirEntries[validEntries].bHeight = grpIconDir->idEntries[i].bHeight;
                     iconDirEntries[validEntries].bColorCount = grpIconDir->idEntries[i].bColorCount;
                     iconDirEntries[validEntries].bReserved = grpIconDir->idEntries[i].bReserved;
                     iconDirEntries[validEntries].wPlanes = grpIconDir->idEntries[i].wPlanes;
                     iconDirEntries[validEntries].wBitCount = grpIconDir->idEntries[i].wBitCount;
-                    iconDirEntries[validEntries].dwBytesInRes = grpIconDir->idEntries[i].dwBytesInRes;
+                    iconDirEntries[validEntries].dwBytesInRes = iconBytes;
                     iconDirEntries[validEntries].dwImageOffset = *iconOffset;
                     validEntries++;
-                    memcpy(&icons[*iconOffset], pIcon, grpIconDir->idEntries[i].dwBytesInRes);
-                    *iconOffset += grpIconDir->idEntries[i].dwBytesInRes;
+                    memcpy(&icons[*iconOffset], pIcon, iconBytes);
+                    *iconOffset += iconBytes;
                 }
                 FreeResource(hResData);
             }