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

["Earle F. Philhower III" <earle at ziplabel dot com>]

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