[PATCH 1/2] shell32/iconcache: Prevent the imagelists from going out of sync by generating invalid icons from available icons
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@gmail.com --- 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;
There is no need to initialize and destroy shortcut icons that we haven't even processed.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/shell32/iconcache.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c index 5ffb293..2e6e0ff 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -354,7 +354,6 @@ static INT SIC_IconAppend (const WCHAR *sourcefile, INT src_index, HICON *hicons 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; INT ret = -1; @@ -421,6 +420,7 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
if (flags & GIL_FORSHORTCUT) { + HICON hshortcuts[ARRAY_SIZE(hicons)]; BOOL failed = FALSE;
for (i = 0; i < ARRAY_SIZE(hshortcuts); i++) @@ -429,13 +429,15 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) { WARN("Failed to create shortcut overlaid icons.\n"); failed = TRUE; + break; } }
if (failed) { - for (i = 0; i < ARRAY_SIZE(hshortcuts); i++) - DestroyIcon(hshortcuts[i]); + unsigned int k; + for (k = 0; k < i; k++) + DestroyIcon(hshortcuts[k]); flags &= ~GIL_FORSHORTCUT; } else
Probably LoadIconWithScaleDown() could be used here, once it works correctly.
Also, is it possible to have a test for this? Loading single icon size, and testing if the remaining sizes are created too?
On Tue, Oct 9, 2018 at 8:47 AM Nikolay Sivov nsivov@codeweavers.com wrote:
Also, is it possible to have a test for this? Loading single icon size, and testing if the remaining sizes are created too?
I'm not sure how to go about making this test. The function SIC_LoadIcon loads all sizes by itself, it's only when PrivateExtractIconsW fails to load only some of the icon sizes (but not all) that this happens. But I don't know how to make PrivateExtractIconsW fail only on some icon sizes but not others, which would be needed for a test.
Also note that PrivateExtractIconsW can return non-zero *even* if the icon failed to load (so it's still NULL). Checking its return value is not reliable at all for this context. I don't know if that's a bug in the function or not, but it's safer this way.
On Tue, Oct 9, 2018 at 8:47 AM Nikolay Sivov nsivov@codeweavers.com wrote:
Also, is it possible to have a test for this? Loading single icon size, and testing if the remaining sizes are created too?
Hi Nikolay,
I need some help with the tests, because I've no idea what executable or file to use that exhibits this behavior, except for TurnItOn.exe (linked in bug report).
Right now, I've got the tests working properly (meaning, it succeeds on Windows XP, but fails on Wine without the patch), but they require TurnItOn.exe, so I have something like:
SHGetFileInfoA("C:\TurnItOn.exe", 0, &info, ...)
Then I check the ImageList_GetImageCount of SHIL_SMALL and SHIL_LARGE and see if they differ (they do on Wine, not on Windows).
Which is obviously not acceptable for wine tests since the executable is not bundled (but is required to exhibit the "out of sync" in the system ImageLists). I'm not aware of any other executable or whatever other file that exhibits this behavior.
What should I do?
-
Gabriel Ivăncescu -
Nikolay Sivov