[PATCH 1/2] shell32/iconcache: Prevent the imagelists from going out of sync when loading an icon
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. Even when PrivateExtractIconsW succeeds, the returned handle can still be NULL, leading to the ImageLists becoming out of sync. This fixes a regression, because at some point, we stopped failing if one icon failed to extract, so when SIC_IconAppend called ImageList_AddIcon with a NULL icon handle, it would go out of sync. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45696 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/iconcache.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c index 6107e0c..44422ab 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -364,13 +364,14 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 }; unsigned int i; SIZE size; - int ret; + INT ret = -1; 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)); + if (!hicons[i]) goto fail; } if (flags & GIL_FORSHORTCUT) @@ -403,6 +404,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; -- 2.19.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 44422ab..b9264bb 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -361,7 +361,6 @@ static BOOL get_imagelist_icon_size(int list, SIZE *size) static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) { HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 }; - HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 }; unsigned int i; SIZE size; INT ret = -1; @@ -376,6 +375,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++) @@ -384,13 +384,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 -- 2.19.1
On Mon, Nov 26, 2018 at 12:22:04PM +0200, Gabriel Ivăncescu wrote:
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 44422ab..b9264bb 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -361,7 +361,6 @@ static BOOL get_imagelist_icon_size(int list, SIZE *size) static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) { HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 }; - HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 }; unsigned int i; SIZE size; INT ret = -1; @@ -376,6 +375,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++) @@ -384,13 +384,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
Rather than optimizing the failure case, we should focus on addressing the real problem. Huw.
On Thu, Nov 29, 2018 at 1:13 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Mon, Nov 26, 2018 at 12:22:04PM +0200, Gabriel Ivăncescu wrote:
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 44422ab..b9264bb 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -361,7 +361,6 @@ static BOOL get_imagelist_icon_size(int list, SIZE *size) static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) { HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 }; - HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 }; unsigned int i; SIZE size; INT ret = -1; @@ -376,6 +375,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++) @@ -384,13 +384,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
Rather than optimizing the failure case, we should focus on addressing the real problem.
Huw.
By real problem, you mean the icon not being reconstructed from the available ones? I planned to do that after, because the patch that does it is a bit convoluted. Now I just sent the easy bits first. I mean, I *totally* understand why that patch feels like a semi-hack. At worst I was planning to get it into wine-staging, at least until we find a better way to do it. BTW, I asked Nikolay before but he didn't answer -- I'm not sure how to make tests for that patch's scenario, because I don't know how to reproduce the failure of only some icon sizes in a wine test (as in the bug report via TurnItOn.exe). I mean, it's trivial to reproduce it with TurnItOn.exe but I can't just ship that with the test obviously.
On Thu, Nov 29, 2018 at 01:17:57PM +0200, Gabriel Ivăncescu wrote:
On Thu, Nov 29, 2018 at 1:13 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Mon, Nov 26, 2018 at 12:22:04PM +0200, Gabriel Ivăncescu wrote:
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 44422ab..b9264bb 100644 --- a/dlls/shell32/iconcache.c +++ b/dlls/shell32/iconcache.c @@ -361,7 +361,6 @@ static BOOL get_imagelist_icon_size(int list, SIZE *size) static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags) { HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 }; - HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 }; unsigned int i; SIZE size; INT ret = -1; @@ -376,6 +375,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++) @@ -384,13 +384,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
Rather than optimizing the failure case, we should focus on addressing the real problem.
Huw.
By real problem, you mean the icon not being reconstructed from the available ones? I planned to do that after, because the patch that does it is a bit convoluted. Now I just sent the easy bits first.
I mean, I *totally* understand why that patch feels like a semi-hack. At worst I was planning to get it into wine-staging, at least until we find a better way to do it.
BTW, I asked Nikolay before but he didn't answer -- I'm not sure how to make tests for that patch's scenario, because I don't know how to reproduce the failure of only some icon sizes in a wine test (as in the bug report via TurnItOn.exe). I mean, it's trivial to reproduce it with TurnItOn.exe but I can't just ship that with the test obviously.
Err, what this patch does is to optimize things when SIC_OverlayShortcutImage() fails (and unnecessarily move around a variable definition). So the real problem would be why that function fails. FWIW the other patch was a bit of a hack, but having the imagelists go out of sync seemed worse than a missing icon, so I signed off on that one. Huw.
On Thu, Nov 29, 2018 at 1:37 PM Huw Davies <huw(a)codeweavers.com> wrote:
Err, what this patch does is to optimize things when SIC_OverlayShortcutImage() fails (and unnecessarily move around a variable definition). So the real problem would be why that function fails.
FWIW the other patch was a bit of a hack, but having the imagelists go out of sync seemed worse than a missing icon, so I signed off on that one.
Huw.
Oh sorry I thought you were speaking of the original (old) patchset where I re-created missing icons from the available ones, which was way more hackish :) My bad. BTW I moved the variable definition, because I like/think it's better to have them as localized as possible which is better for scope encapsulation, it's just a coding style I follow (mostly from C++ projects). So I'm asking for future now if I should do this kind of thing or it's not welcome in Wine policy unless I actually change more of something or refactor a function?
On Thu, Nov 29, 2018 at 01:50:16PM +0200, Gabriel Ivăncescu wrote:
BTW I moved the variable definition, because I like/think it's better to have them as localized as possible which is better for scope encapsulation, it's just a coding style I follow (mostly from C++ projects). So I'm asking for future now if I should do this kind of thing or it's not welcome in Wine policy unless I actually change more of something or refactor a function?
Generally if it's new code you can put them where you want (within reason) although various area maintainers may have specific requests. When making alterations to old code, adding needless style changes makes the patch size larger and therefore harder to review. The goal (as I've said before) is to make it easy for the reviewer. Huw.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies