[Bug 25691] New: ImageList_Duplicate doesn't correctly duplicate full length of has_alpha byte array

http://bugs.winehq.org/show_bug.cgi?id=25691

Summary: ImageList_Duplicate doesn't correctly duplicate full
                    length of has_alpha byte array
           Product: Wine
           Version: unspecified
          Platform: x86
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: comctl32
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

In the case where an image list is created, then expanded enough so that the
byte array pointed at by the has_alpha member has been reallocated to a larger
size (and cMaxImage is larger), if that image list is duplicated, the duplicate
will have a has_alpha member that is shorter than the expected length.

Where ImageList_Create assigns:

himl->cMaxImage = cInitial + 1;

and then ensures the length of the has_alpha byte array is at least cMaxImage
bytes long,

ImageList_Duplicate assigns to the duplicate:

himlDst->cMaxImage = himlSrc->cMaxImage;

which throws off the internal checks that assume the length of the has_alpha
member is at least as long as cMaxImage says.  This results in heap corruption
when the following line in add_with_alpha() is executed under the right
conditions:

himl->has_alpha[pos + n] = 1;

I worked around the bug by making the following changes to
dlls/comctl32/imagelist.c:

*** 1519,1524 ****
--- 1519,1537 ----
                      himlSrc->hdcMask, 0, 0, SRCCOPY);

himlDst->cCurImage = himlSrc->cCurImage;
+         if (himlSrc->has_alpha && himlDst->has_alpha
+             && himlSrc->cMaxImage > himlDst->cMaxImage)
+         {
+             /* ImageList_Create didn't create a long enough new_alpha */
+             char *new_alpha = HeapReAlloc( GetProcessHeap(),
HEAP_ZERO_MEMORY,
+                 himlDst->has_alpha, himlSrc->cMaxImage );
+             if (new_alpha) himlDst->has_alpha = new_alpha;
+             else
+             {
+                 HeapFree( GetProcessHeap(), 0, himlDst->has_alpha );
+                 himlDst->has_alpha = NULL;
+             }
+         }
      himlDst->cMaxImage = himlSrc->cMaxImage;
          if (himlSrc->has_alpha && himlDst->has_alpha)
              memcpy( himlDst->has_alpha, himlSrc->has_alpha,
himlDst->cCurImage );

I built Wine 1.2.2 from source (./configure; make; sudo make install), and then
applied the changes the same way and retested.

Looks like the 1.3 and development lines have the same issue.

I don't know if this patch is efficient enough for me to submit it (nicer if
ImageList_Create would accept cMaxImage as a parameter so it could create the
byte array with the correct length to save an allocation, but that's a bigger
change).