Bug#629611: xserver-xorg: reproducible X server segfault

[pacman@kosh.dhis.org]

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.

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.

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.

Demo program crashx.c follows

=== CUT HERE ===
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/shm.h>
#include <X11/Xlib.h>
#include <X11/extensions/XShm.h>

/* How to use:
     cc crashx.c -lX11 -lXext -o crashx
     Xvfb :1 &
     sleep 5
     DISPLAY=:1 ./crashx 256 256
   Enjoy! */

int main(int argc, char **argv)
{
  int width, height, pad, paddedwidth, imagesize;
  char *end;
  Display *dpy;
  int scr;
  XSetWindowAttributes attr;
  Window w;
  XEvent ev;
  XGCValues gcv;
  GC gc;
  XImage *ximage;
  XShmSegmentInfo xshm;

  if(argc!=3 || !(width=strtoul(argv[1], &end, 10)) || *end
             || !(height=strtoul(argv[2], &end, 10)) || *end) {
    fprintf(stderr, "Usage: %s width height\n", argv[0]);
    return 2;
  }

  dpy = XOpenDisplay(0);
  if(!dpy) {
    fprintf(stderr, "XOpenDisplay failed. Is $DISPLAY OK?\n");
    return 1;
  }

  pad = BitmapPad(dpy);
  paddedwidth = (width+pad-1)/pad*pad;
  imagesize = paddedwidth*height;
  if(imagesize/height != paddedwidth) {
    fprintf(stderr, "Too big!\n");
    return 1;
  }
  imagesize /= 8;
  fprintf(stderr, "width padded to %d, image size %d bytes\n",
          paddedwidth, imagesize);

  scr = DefaultScreen(dpy);

  attr.event_mask = ExposureMask;

  w = XCreateWindow(dpy, DefaultRootWindow(dpy), 0, 0, width, height,
                    0, DefaultDepth(dpy, scr), InputOutput, 
                    DefaultVisual(dpy, scr), CWEventMask, &attr);

  XMapWindow(dpy, w);

  gcv.foreground = BlackPixel(dpy, scr);
  gcv.background = WhitePixel(dpy, scr);
  gc = XCreateGC(dpy, w, GCForeground | GCBackground, &gcv);

  xshm.shmid = shmget(IPC_PRIVATE, imagesize, IPC_CREAT|0777);
  if(xshm.shmid<0) {
    perror("shmget");
    return 1;
  }

  xshm.shmaddr = shmat(xshm.shmid, 0, 0);
  if(xshm.shmaddr==(void *)-1) {
    perror("shmat");
    return 1;
  }
  memset(xshm.shmaddr, 0, sizeof imagesize);

  xshm.readOnly = False;

  XShmAttach(dpy, &xshm);
  ximage = XShmCreateImage(dpy, DefaultVisual(dpy, scr), 1, XYBitmap,
                           xshm.shmaddr, &xshm, width, height);
  XSync(dpy, False);
  shmdt(xshm.shmaddr);
  shmctl(xshm.shmid, IPC_RMID, 0);

  while(1) {
    XNextEvent(dpy, &ev);
    if(ev.type != Expose)
      continue;
    XShmPutImage(dpy, w, gc, ximage, 0, 0, 0, 0, width-1, height, False);
  }
}
=== CUT HERE ===

-- 
Alan Curry