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

Nikolay Sivov nsivov at codeweavers.com
Wed Jun 20 02:49:57 CDT 2018


On 06/16/2018 07:19 PM, Denis Malikov wrote:

> From: Denis Malikov <mdn40000 at mail.ru>
> 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 <mdn40000 at mail.ru>
> ---
>   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?



More information about the wine-devel mailing list