XWin 4.3.0-50 crashes with -multiwindow (ping Earle)

[Earle F. Philhower III]

Howdy Fabrizio, Harold.

Thanks for the good debug Fabrizio!  The 24bpp icon handling was
something I never could test: I couldn't find any apps that had
24bpp icons, all I found were 1- 15-, 16-, or 32-bit ones.
I was assuming the X server always used a packed format, but
PixmapBytePad() looks to be the proper way of handling this.
(Can I ask how you knew?  I did a Google on the macro and didn't
come up with anything of interest, I only found stuff in the
header files themselves...)

Harold, the xStride calc looks great, but by removing the conversion
of 15-bpp modes into effective 16-bpp modes will break 15bpp icon
handling.  15-bpp modes are handled exactly the same as 16bpp modes,
except they are not bit-packed (there's 1 extra unused bit every
pixel) so you can't do (bpp/8).

To fix it we can reinstate the if()...
 >   if (pixmap->drawable.bitsPerPixel == 15)
 >     effXBPP = 16;
 >   else
 >     effXBPP = pixmap->drawable.bitsPerPixel;
 >   if (pixmap->drawable.depth == 15)
 >     effXDepth = 16;
 >   else
 >     effXDepth = pixmap->drawable.depth;
Or get rid of the effX* variables completely, but modify (~line 218)
< if (effxdepth==16) into
into
 > if (xdepth==16 || xdepth==15)

and modify all of the X image ptr walking
<             ptr += posX * (effXBPP / 8);
into
 >             ptr += (xbpp==15)?(posX * (16/8):(posX * (xbpp/ 8));

If you'd like I can do it, just let me know which one would be
more appealing!  Personally I find the effX variables make the routine
a bit more readable, but that's kind of expected since I put them
there in the first place...



At 07:49 PM 3/23/2004 -0500, you wrote:
>Fabrizio,
>
>It looks like your conclusions are correct.
>
>I have included your suggested change in XFree86-xserv-4.3.0-60.  Please 
>test this on a 24 bit depth system.  It seems to work okay on 32 bit depth 
>systems.
>
>I checked the change into CVS as well.  I think that Earle should probably 
>take a look at it to verify that it is likely correct since there are 
>several calculations to correct the depth and bpp values, which may be 
>made redundant by just using PixmapBytePad instead; but the function 
>confuses me too much to understand it quickly.
>
>Harold
>
>wrote:
>
>>Harold and all,
>>I built XWin from source and debugged with gdb, and in that way I was able
>>to track down the bug. It is due to my visual being 24bpp. It does not occur
>>if it is changed to 16bpp. 32bpp is not available, but I am confident 
>>everything
>>would work in that case.
>>Here is what happens:
>>- winScaleXBitmapToWindows is called. The pixmap passed has height 42, width
>>48 and bitsPerPixel 24
>>- effXBPP is 24, xStride is 144 (48*(24/8))
>>- iconData is allocated as an array of 144*42 bytes
>>- then, miGetImage is called. Here the line
>>linelength = PixmapBytePad(w, depth);
>>is executed with w=48 and depth=24. As a result, linelength is 192 (48*4),
>>not 144 (48*3).
>>- in the following for cycle, pDst (initialized as iconData) is incremented
>>by linelength(=192) each time. Soon the pointer overflows the allocated
>>bounds, causing the crash.
>>It seems that handling of 24-bit display is broken. Maybe 
>>winScaleXBitmapToWindows
>>should use PixmapBytePad to calculate xStride, but I'm only guessing as
>>I'm not an expert.
>>Regards,
>>Fabrizio
>
>-Earle F. Philhower, III
>  earle@ziplabel.com
>  cdrlabel - ZipLabel - FlpLabel
>  http://www.cdrlabel.com