[PATCH v2 1/3] shell32/iconcache: Prevent the imagelists from going out of sync by generating invalid icons from available icons

[Gabriel Ivăncescu]

When populating the imagelists cache with icons of different sizes, it
can happen that only some icon sizes actually succeed the extraction,
with others failing with a NULL handle. This is especially true with some
broken "executable packers" where for example, a 32x32 icon loads okay,
but a 16x16 fails. Therefore, for icons that failed to load, create them
from another icon that succeeded by resizing it, favoring icons that are
the closest and larger (to reduce pixelation artefacts) and with the closest
aspect ratio as the source of this operation (to be as generic as possible).

For example, if the icon that needs to be created must be 16x16, an 18x18 icon
would get picked over either a 32x32 (it's further from 16x16) or a 15x15
(icons larger than 16x16 are favored since they're larger than the result,
so smaller icons are only picked if no other available icon is larger).

This also fixes a regression, because at some point, we stopped failing
if one icon failed to extract (even if PrivateExtractIconsW returned
success, icon handle could still be NULL), so when SIC_IconAppend called
ImageList_AddIcon with a NULL icon handle, it would go out of sync and the
icons would be totally screwed up until the application is restarted.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45696
Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
---

v2: Add test.

 dlls/shell32/iconcache.c | 72 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c
index 6107e0c..5ffb293 100644
--- a/dlls/shell32/iconcache.c
+++ b/dlls/shell32/iconcache.c
@@ -345,13 +345,6 @@ static INT SIC_IconAppend (const WCHAR *sourcefile, INT src_index, HICON *hicons
     return ret;
 }
 
-static BOOL get_imagelist_icon_size(int list, SIZE *size)
-{
-    if (list < 0 || list >= ARRAY_SIZE(shell_imagelists)) return FALSE;
-
-    return ImageList_GetIconSize( shell_imagelists[list], &size->cx, &size->cy );
-}
-
 /****************************************************************************
  * SIC_LoadIcon				[internal]
  *
@@ -362,15 +355,68 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
 {
     HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 };
     HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 };
+    SIZE size[ARRAY_SIZE(shell_imagelists)];
     unsigned int i;
-    SIZE size;
-    int ret;
+    INT ret = -1;
 
+    /* Keep track of the sizes in case any icon fails to get extracted */
     for (i = 0; i < ARRAY_SIZE(hicons); i++)
     {
-        get_imagelist_icon_size( i, &size );
-        if (!PrivateExtractIconsW( sourcefile, index, size.cx, size.cy, &hicons[i], 0, 1, 0 ))
-            WARN("Failed to load icon %d from %s.\n", index, debugstr_w(sourcefile));
+        ImageList_GetIconSize(shell_imagelists[i], &size[i].cx, &size[i].cy);
+        PrivateExtractIconsW(sourcefile, index, size[i].cx, size[i].cy, &hicons[i], 0, 1, 0);
+    }
+
+    /* Fill any icon handles that failed to get extracted, by resizing
+       another icon handle that succeeded and creating the icon from it.
+       Use a dumb O(n^2) algorithm since ARRAY_SIZE(hicons) is small */
+    for (i = 0; i < ARRAY_SIZE(hicons); i++)
+    {
+        unsigned int k, ix, iy;
+        BOOL failed = TRUE;
+        if (hicons[i]) continue;
+
+        for (k = 0; k < ARRAY_SIZE(hicons); k++)
+        {
+            if (hicons[k])
+            {
+                ix = iy = k;
+                failed = FALSE;
+                break;
+            }
+        }
+        if (failed) goto fail;
+
+        for (k++; k < ARRAY_SIZE(hicons); k++)
+        {
+            if (!hicons[k]) continue;
+
+            /* Find closest-sized icon, but favor larger icons to resize from */
+            if (size[k].cx >= size[i].cx)
+                ix = (size[ix].cx < size[i].cx || size[ix].cx > size[k].cx) ? k : ix;
+            else
+                ix = (size[ix].cx < size[i].cx && size[ix].cx < size[k].cx) ? k : ix;
+
+            if (size[k].cy >= size[i].cy)
+                iy = (size[iy].cy < size[i].cy || size[iy].cy > size[k].cy) ? k : iy;
+            else
+                iy = (size[iy].cy < size[i].cy && size[iy].cy < size[k].cy) ? k : iy;
+        }
+
+        /* Use the closest icon in aspect ratio if ix and iy differ */
+        if (ix != iy)
+        {
+            float i_ratio, ix_ratio, iy_ratio;
+            i_ratio  = (float)size[i].cx  / (float)size[i].cy;
+            ix_ratio = (float)size[ix].cx / (float)size[ix].cy;
+            iy_ratio = (float)size[iy].cx / (float)size[iy].cy;
+            if (fabsf(ix_ratio - i_ratio) > fabsf(iy_ratio - i_ratio))
+                ix = iy;
+        }
+
+        /* If this fails, we have to abort to prevent the image lists from
+           becoming out of sync and completely screwing the icons up */
+        hicons[i] = CopyImage(hicons[ix], IMAGE_ICON, size[i].cx, size[i].cy, 0);
+        if (!hicons[i]) goto fail;
     }
 
     if (flags & GIL_FORSHORTCUT)
@@ -403,6 +449,8 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
     }
 
     ret = SIC_IconAppend( sourcefile, index, hicons, flags );
+
+fail:
     for (i = 0; i < ARRAY_SIZE(hicons); i++)
         DestroyIcon(hicons[i]);
     return ret;
-- 
1.9.1