Bug#629611: xserver-xorg: reproducible X server segfault
On Wed, Nov 2, 2011 at 17:58:23 -0500, pacman@kosh.dhis.org wrote:
> I'm including here an updated version of the bug demonstration program. It
> now reliably segfaults every X server I can find. (That's not a lot of them
> since I haven't looked outside Debian stable.)
>
> It does the same basic operations as the previous test program in its
> bug-triggering mode, but allows width and height of the window to be adjusted
> via command line parameters.
>
> The bug involves the server reading past the end of the image data, so the
> chance of segfault increases when there is no space between the end of the
> image data and the end of the shm segment. A 256x256 bitmap takes up exactly
> 8192 bytes so it works very well. If 256x256 doesn't do it, I suggest 32768x1
> as a likely alternative.
>
Thanks for the test case. I can't make it crash Xvfb or Xephyr when
running them directly, but they reproducibly die under valgrind.
> With the help of this crash-generating client, I've been studying the crash
> in gdb, using an Xvfb compiled -O0. The story is basically told by the
> backtrace I sent earlier in this bug's history. fb24_32CopyMtoN is being
> called with a 1bpp source and a 32bpp destination. That's bad, since it
> only expects to be doing a 24-to-32 or 32-to-24 conversion.
>
> It gets to fb24_32CopyMtoN because fbCopyArea calls that whenever the source
> and destination have different bpp. I can only assume that fbCopyArea is not
> supposed to be called with arbitrarily different bpp. So the caller made a
> mistake.
>
Yup, CopyArea takes drawables with matching depths AIUI.
> Continuing up the backtrace, we see various extensions which aren't doing
> anything relevant to the bug. Above those is doShmPutImage from Xext/shm.c
> which has called pGC->ops->CopyArea. If the rule for calling
> pGC->ops->CopyArea is anything like the rule for using the CopyArea request
> in the base protocol, then it's an error to call it with 2 objects that don't
> have the same depth. But if that's the case, then I can't figure out why an
> fb24_32CopyMtoN would ever be needed in fbCopyArea.
>
> Looking into the history of doShmPutImage, I found commit
> 11817a881cb93a89788105d1e575a468f2a8d27c "Fix ShmPutImage non-ZPixmap case."
> which created the current top-level structure of doShmPutImage in which the
> src depth==1 case is explicitly routed into CopyArea. I don't understand how
> that could be correct, yet it claims to be a fix for something very similar
> to the current problem.
>
> The comment above doShmPutImage is also confusing:
> /*
> * If the given request doesn't exactly match PutImage's constraints,
> * wrap the image in a scratch pixmap header and let CopyArea sort it out.
> */
> As far as I can tell, the problem is that CopyArea has a constraint requiring
> matching depths, which PutImage doesn't. So punting the hard case to CopyArea
> is exactly the opposite of the right thing to do.
>
Thanks for the detective work.
Michel, would this make sense, to avoid calling CopyArea for an XYBitMap
image?
diff --git a/Xext/shm.c b/Xext/shm.c
index a6f804c..223935e 100644
--- a/Xext/shm.c
+++ b/Xext/shm.c
@@ -498,7 +498,7 @@ doShmPutImage(DrawablePtr dst, GCPtr pGC,
{
PixmapPtr pPixmap;
- if (format == ZPixmap || depth == 1) {
+ if (format == ZPixmap || (format == XYPixmap && depth == 1)) {
pPixmap = GetScratchPixmapHeader(dst->pScreen, w, h, depth,
BitsPerPixel(depth),
PixmapBytePad(w, depth),
Cheers,
Julien
Reply to: