[PATCH v2 1/3] 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(a)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
There is no need to initialize and destroy shortcut icons that we haven't even processed. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)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 -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- test.ico is just a black icon of size 32x32. dlls/shell32/tests/rsrc.rc | 2 ++ dlls/shell32/tests/shelllink.c | 43 +++++++++++++++++++++++++++++++++++++++++ dlls/shell32/tests/test.ico | Bin 0 -> 3262 bytes 3 files changed, 45 insertions(+) create mode 100644 dlls/shell32/tests/test.ico diff --git a/dlls/shell32/tests/rsrc.rc b/dlls/shell32/tests/rsrc.rc index 00c949b..0706028 100644 --- a/dlls/shell32/tests/rsrc.rc +++ b/dlls/shell32/tests/rsrc.rc @@ -26,3 +26,5 @@ STRINGTABLE { 1 "Folder Name Resource" } + +1 ICON "test.ico" diff --git a/dlls/shell32/tests/shelllink.c b/dlls/shell32/tests/shelllink.c index 3bfd9cb..4feb5dd 100644 --- a/dlls/shell32/tests/shelllink.c +++ b/dlls/shell32/tests/shelllink.c @@ -47,6 +47,7 @@ static HRESULT (WINAPI *pSHGetStockIconInfo)(SHSTOCKICONID, UINT, SHSTOCKICONINF static DWORD (WINAPI *pGetLongPathNameA)(LPCSTR, LPSTR, DWORD); static DWORD (WINAPI *pGetShortPathNameA)(LPCSTR, LPSTR, DWORD); static UINT (WINAPI *pSHExtractIconsW)(LPCWSTR, int, int, int, HICON *, UINT *, UINT, UINT); +static DWORD_PTR (WINAPI *pSHGetFileInfoW)(LPCWSTR, DWORD, SHFILEINFOW *, UINT, UINT); static BOOL (WINAPI *pIsProcessDPIAware)(void); static const GUID _IID_IShellLinkDataList = { @@ -1477,6 +1478,46 @@ static void test_SHGetImageList(void) } } +static void test_ImageList_sync(void) +{ + static const UINT ImageList_size[] = { SHIL_LARGE, SHIL_SMALL }; + int count[ARRAY_SIZE(ImageList_size)]; + WCHAR buf[MAX_PATH]; + SHFILEINFOW info; + DWORD nr; + UINT i; + + if (!pSHGetFileInfoW) + { + win_skip("SHGetFileInfoW not available\n"); + return; + } + + nr = GetModuleFileNameW(NULL, buf, ARRAY_SIZE(buf)); + ok(nr, "GetModuleFileName failed, last error %08x\n", GetLastError()); + if (!nr) return; + + nr = pSHGetFileInfoW(buf, 0, &info, sizeof(info), SHGFI_ICON); + ok(nr, "SHGetFileInfo returned 0, last error %08x\n", GetLastError()); + if (!nr) return; + + ok(info.hIcon != NULL, "NULL icon\n"); + DestroyIcon(info.hIcon); + + for (i = 0; i < ARRAY_SIZE(ImageList_size); i++) + { + IImageList *il; + HRESULT hr; + + hr = SHGetImageList(ImageList_size[i], &IID_IImageList, (void**)&il); + ok(hr == S_OK, "got %08x\n", hr); + hr = IImageList_GetImageCount(il, &count[i]); + ok(hr == S_OK, "got %08x\n", hr); + IImageList_Release(il); + } + ok(count[0] == count[1], "ImageLists out of sync (large: %d, small: %d)\n", count[0], count[1]); +} + START_TEST(shelllink) { HRESULT r; @@ -1493,6 +1534,7 @@ START_TEST(shelllink) pGetLongPathNameA = (void *)GetProcAddress(hkernel32, "GetLongPathNameA"); pGetShortPathNameA = (void *)GetProcAddress(hkernel32, "GetShortPathNameA"); pSHExtractIconsW = (void *)GetProcAddress(hmod, "SHExtractIconsW"); + pSHGetFileInfoW = (void *)GetProcAddress(hmod, "SHGetFileInfoW"); pIsProcessDPIAware = (void *)GetProcAddress(huser32, "IsProcessDPIAware"); r = CoInitialize(NULL); @@ -1511,6 +1553,7 @@ START_TEST(shelllink) test_ExtractIcon(); test_ExtractAssociatedIcon(); test_SHGetImageList(); + test_ImageList_sync(); CoUninitialize(); } diff --git a/dlls/shell32/tests/test.ico b/dlls/shell32/tests/test.ico new file mode 100644 index 0000000000000000000000000000000000000000..88ad5974402a9aa222899f05390fb6ada6067135 GIT binary patch literal 3262 zcmeIup%DNe5Cg%(qZ#B&%Aj1zBvgU9|4BsTLd#c?o}3N?SV<;!@^()F1r$&~0R<FL LKmi35_$_b&hYA75 literal 0 HcmV?d00001 -- 1.9.1
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=43673 Your paranoid android. === wvistau64_fr (32 bit Windows report) === shell32: brsfolder: Timeout === w7u (32 bit Windows report) === shell32: progman_dde.c:330: Test failed: window not created === w7pro64 (task log) === Task errors: TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally === w864 (task log) === Task errors: TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally === w2003std (32 bit Windows report) === shell32: shlfolder.c:4955: Test failed: MKDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 8) shlfolder.c:4955: Test failed: CREATE: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 2) shlfolder.c:4955: Test failed: RMDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 10) shlfolder.c:4955: Test failed: MKDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac) shlfolder.c:4955: Test failed: CREATE: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac) shlfolder.c:4955: Test failed: RMDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac) === debian9 (64 bit Wow Wine report) === shell32: shelllink.c:761: Test failed: save failed (0x80070020) shelllink.c:761: Test failed: got 0x00000001 shelllink.c:761: Test failed: Didn't expect NULL shelllink.c:507: Test failed: GetCurFile fails on shell32 < 5.0 shelllink.c:765: Test failed: GetDescription returned 'command on path' instead of 'command on path without .exe' shelllink.c:765: Test succeeded inside todo block: GetPath returned 'C:\windows\system32\rundll32.exe' instead of 'C:\windows\system32\rundll32.exe' === debian9 (build log) ===
Pre-existing failures. Not the fault of this patch, please ignore. On Tue, Oct 30, 2018 at 3:52 PM Marvin <testbot(a)winehq.org> wrote:
Hi,
While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=43673
Your paranoid android.
=== wvistau64_fr (32 bit Windows report) ===
shell32: brsfolder: Timeout
=== w7u (32 bit Windows report) ===
shell32: progman_dde.c:330: Test failed: window not created
=== w7pro64 (task log) ===
Task errors: TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally
=== w864 (task log) ===
Task errors: TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally
=== w2003std (32 bit Windows report) ===
shell32: shlfolder.c:4955: Test failed: MKDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 8) shlfolder.c:4955: Test failed: CREATE: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 2) shlfolder.c:4955: Test failed: RMDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 10) shlfolder.c:4955: Test failed: MKDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac) shlfolder.c:4955: Test failed: CREATE: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac) shlfolder.c:4955: Test failed: RMDIR: Expected wndproc to be called shlfolder.c:4867: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 6ac)
=== debian9 (64 bit Wow Wine report) ===
shell32: shelllink.c:761: Test failed: save failed (0x80070020) shelllink.c:761: Test failed: got 0x00000001 shelllink.c:761: Test failed: Didn't expect NULL shelllink.c:507: Test failed: GetCurFile fails on shell32 < 5.0 shelllink.c:765: Test failed: GetDescription returned 'command on path' instead of 'command on path without .exe' shelllink.c:765: Test succeeded inside todo block: GetPath returned 'C:\windows\system32\rundll32.exe' instead of 'C:\windows\system32\rundll32.exe'
=== debian9 (build log) ===
participants (2)
-
Gabriel Ivăncescu -
Marvin