[PATCH 1/2] shell32/iconcache: Prevent the imagelists from going out of sync when loading an icon

[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. 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 
---
 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;

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Gabriel Ivăncescu]

There is no need to initialize and destroy shortcut icons that we haven't
even processed.

Signed-off-by: Gabriel Ivăncescu 
---
 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

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Huw Davies]

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
>
> 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.

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Gabriel Ivăncescu]

On Thu, Nov 29, 2018 at 1:13 PM Huw Davies  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
> >
> > 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.

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Huw Davies]

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  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
> > >
> > > 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.

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Gabriel Ivăncescu]

On Thu, Nov 29, 2018 at 1:37 PM Huw Davies  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?

[PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Huw Davies]

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.

[Huw Davies]

Signed-off-by: Huw Davies