Re: [PATCH] comctl32/imagelist: fix ImageList_Read/Write.

[Nikolay Sivov]

On 06/16/2018 07:19 PM, Denis Malikov wrote:
> From: Denis Malikov 
> Date: Sat, 16 Jun 2018 16:35:45 +0700
> Subject: [PATCH] comctl32/imagelist: fix ImageList_Read/Write.
>
> Fix for versions x600 and x620 and pointer calculation for mixing image and mask bits.
>
> Tested on ReactOS 0.4.10 with *.reg files extracted from:
> - XP/2003 key HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\TrayNotify;
> - Vista/7 key HKEY_CLASSES_ROOT\Local Settings\Software\Microsoft\Windows\CurrentVersion\TrayNotify
> Signed-off-by: Denis Malikov
>
> dlls/comctl32/imagelist.c | 59 +++++++++++++++++++++++++++--------------------
>   1 file changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/dlls/comctl32/imagelist.c b/dlls/comctl32/imagelist.c
> index a08d60752e..052d59f9e4 100644
> --- a/dlls/comctl32/imagelist.c
> +++ b/dlls/comctl32/imagelist.c
> @@ -59,7 +59,7 @@ struct _IMAGELIST
>       INT         cGrow;                     /* 0C: cGrow */
>       INT         cx;                        /* 10: cx */
>       INT         cy;                        /* 14: cy */
> - DWORD       x4;
> - DWORD       x4;                        /* hack for IL from stream. Keep version here */
>  UINT        flags;                     /* 1C: flags */
>  COLORREF    clrFg;                     /* 20: foreground color */
>  COLORREF    clrBk;                     /* 24: background color */
>
> @@ -83,6 +83,7 @@ struct _IMAGELIST
>   };

The idea is to keep this structure binary compatible, that's why it 
starts with a part with specific layout.
If you want to store source stream version, append a new field for that. 
I actually like the idea of using
source stream format version to save the list back, it seems like a good 
compromise.
> #define IMAGELIST_MAGIC 0x53414D58
> +#define IMAGELIST_VERSION 0x101
>
> /* Header used by ImageList_Read() and ImageList_Write() */
>   #include "pshpack2.h"
> @@ -806,6 +807,7 @@ ImageList_Create (INT cx, INT cy, UINT flags,
>       himl->clrFg     = CLR_DEFAULT;
>       himl->clrBk     = CLR_NONE;
>       himl->color_table_set = FALSE;
> -
>
> himl->x4        = 0;
>
> /* initialize overlay mask indices */
>  for (nCount = 0; nCount < MAX_OVERLAYIMAGE; nCount++)
>
> @@ -2257,38 +2259,43 @@ HIMAGELIST WINAPI ImageList_Read(IStream *pstm)
>       BITMAPINFO *image_info = (BITMAPINFO *)image_buf;
>       BITMAPINFO *mask_info = (BITMAPINFO *)mask_buf;
>       void *image_bits, *mask_bits = NULL;
> - ILHEAD	ilHead;
> - HIMAGELIST	himl;
> -
>
> ILHEAD ilHead;
> -
>
> HIMAGELIST himl;
>  unsigned int i;
>
> TRACE("%p\n", pstm);
>
> if (FAILED(IStream_Read (pstm, &ilHead, sizeof(ILHEAD), NULL)))
> - return NULL;
> -
>    return NULL;
> if (ilHead.usMagic != (('L' << 8) | 'I'))
> - return NULL;
> - if (ilHead.usVersion != 0x101) /* probably version? */
> - return NULL;
> -
>    return NULL;

No need to reformat existing lines.
> - if (ilHead.usVersion != IMAGELIST_VERSION &&
> -
>    ilHead.usVersion != 0x600 && /* XP/2003 version */
> -
>    ilHead.usVersion != 0x620)   /* Vista/7 version */
> -
>    return NULL;

The question is if there's any format differences between 0x600 and 
0x620, it was probably bumped for a reason.
>   TRACE("cx %u, cy %u, flags 0x%04x, cCurImage %u, cMaxImage %u\n",
>         ilHead.cx, ilHead.cy, ilHead.flags, ilHead.cCurImage, ilHead.cMaxImage);
>
>   himl = ImageList_Create(ilHead.cx, ilHead.cy, ilHead.flags, ilHead.cCurImage, ilHead.cMaxImage);
>   if (!himl)
> - return NULL;
> -
>    return NULL;
> -
> - // keep version from stream
> - himl->x4 = ilHead.usVersion;

No cpp-style comments, please.
> @@ -3078,10 +3087,10 @@ BOOL WINAPI ImageList_Write(HIMAGELIST himl, IStream *pstm)
>       TRACE("%p %p\n", himl, pstm);
>   if (!is_valid(himl))
> - return FALSE;
> -
>    return FALSE;
>
> ilHead.usMagic   = (('L' << 8) | 'I');
> - ilHead.usVersion = 0x101;
> - ilHead.usVersion = himl->x4 > 0 ? himl->x4 : IMAGELIST_VERSION;
>  ilHead.cCurImage = himl->cCurImage;
>  ilHead.cMaxImage = himl->cMaxImage;
>  ilHead.cGrow     = himl->cGrow;
>
> @@ -3090,23 +3099,23 @@ BOOL WINAPI ImageList_Write(HIMAGELIST himl, IStream *pstm)
>       ilHead.bkcolor   = himl->clrBk;
>       ilHead.flags     = himl->flags;
>       for(i = 0; i < 4; i++) {
> - ilHead.ovls[i] = himl->nOvlIdx[i];
> -
>    ilHead.ovls[i] = himl->nOvlIdx[i];
> }
>
> TRACE("cx %u, cy %u, flags 0x04%x, cCurImage %u, cMaxImage %u\n",
>       ilHead.cx, ilHead.cy, ilHead.flags, ilHead.cCurImage, ilHead.cMaxImage);
>
> if(FAILED(IStream_Write(pstm, &ilHead, sizeof(ILHEAD), NULL)))
> - return FALSE;
> -
>    return FALSE;
>
> /* write the bitmap */
> if(!_write_bitmap(himl->hbmImage, pstm))
> - return FALSE;
> -
>    return FALSE;
>
> /* write the mask if we have one */
> if(himl->flags & ILC_MASK) {
> - if(!_write_bitmap(himl->hbmMask, pstm))
> -
>    return FALSE;
> -
>    if(!_write_bitmap(himl->hbmMask, pstm))
> -
>        return FALSE;
> }

Doesn't this write in old format, with mismatching version field?