[PATCH xserver] xwayland: avoid a crash with empty window pixmaps

[Olivier Fourdan]

Hey Adam,

On 22 January 2018 at 19:57, Adam Jackson <ajax at nwnk.net> wrote:

> On Thu, 2018-01-18 at 11:41 +0100, Olivier Fourdan wrote:
> > This is a rare occurrence of a crash in Xwayland for which I don't have
> > the reproducing steps, just a core file.
> >
> > The backtrace looks as follow:
> >
> >  [...]
> >
> > The crash is caused by dereferencing “xwl_pixmap->buffer” in
> > xwl_glamor_pixmap_get_wl_buffer() because “xwl_pixmap” is NULL.
> >
> > Reason for this is because the corresponding pixmap has a size of 0×0
> > and no xwl_pixmap is created for pixmaps of size 0×0.
>
> That can't really be the problem. X drawables are never 0x0.
>

Yeap, I don't know how we end with a pximap of size 0×0:

(gdb) bt
#0  0x00007f00239a31a7 in __GI_raise (sig=sig at entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007f00239a4898 in __GI_abort () at abort.c:90
#2  0x000000000058f1da in OsAbort () at utils.c:1361
#3  0x0000000000594ce3 in AbortServer () at log.c:877
#4  0x0000000000595b2d in FatalError (f=f at entry=0x5b7490 "Caught signal %d
(%s). Server aborting\n") at log.c:1015
#5  0x000000000058c43c in OsSigHandler (signo=11, sip=<optimized out>,
unused=<optimized out>) at osinit.c:154
#6  <signal handler called>
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap at entry=0x1e5b6f0) at
xwayland-glamor.c:162
#8  0x0000000000424da5 in xwl_screen_post_damage (xwl_screen=0x161d750) at
xwayland.c:514
#9  block_handler (data=0x161d750, timeout=<optimized out>) at
xwayland.c:665
#10 0x0000000000557e46 in BlockHandler (pTimeout=pTimeout at entry=0x7ffeb514bf54)
at dixutils.c:388
#11 0x0000000000585ed9 in WaitForSomething (are_ready=0) at WaitFor.c:219
#12 0x00000000005531e1 in Dispatch () at dispatch.c:422
#13 0x000000000055744a in dix_main (argc=11, argv=0x7ffeb514c138,
envp=<optimized out>) at main.c:287
#14 0x00007f002398f377 in __libc_start_main (main=0x4240d0 <main>, argc=11,
ubp_av=0x7ffeb514c138, init=<optimized out>, fini=<optimized out>,
    rtld_fini=<optimized out>, stack_end=0x7ffeb514c128) at
../csu/libc-start.c:274
#15 0x00000000004240fe in _start ()

(gdb) f 7
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap at entry=0x1e5b6f0) at
xwayland-glamor.c:162
162     if (xwl_pixmap->buffer)

(gdb) p *pixmap
$1 = {drawable = {type = 1 '\001', class = 0 '\000', depth = 24 '\030',
bitsPerPixel = 32 ' ', id = 0, x = 0, y = 0, width = 0, height = 0,
    pScreen = 0x161d200, serialNumber = 1}, devPrivates = 0x1e5b738, refcnt
= 1, devKind = 0, devPrivate = {ptr = 0x1e5b7c0, val = 31832000,
    uval = 31832000, fptr = 0x1e5b7c0}, screen_x = 0, screen_y = 0,
usage_hint = 0, master_pixmap = 0x0}

How we end up here is unclear though, xwl_pixmap is “optimized out” but
considering it's a segfault I assume it's NULL.

If we also assume the pixmap was of size 0×0 when
xwl_glamor_create_pixmap() was called, then we wouldn't be calling
xwl_glamor_create_pixmap_for_bo() which would not call
xwl_pixmap_set_private():


https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland-glamor.c#n183

Instead we would call glamor_create_pixmap() which invokes fbCreatePixmap()
for pixmaps of size 0×0:

  https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor.c#n180

And the values found in the core file match what is set for by
fbCreatePixmap():

 https://cgit.freedesktop.org/xorg/xserver/tree/fb/fbpixmap.c#n31

So that matches.

Also that doesn't look like a use after free (As Daniel said), because the
pixmap has a refcnt =1 and also because on unrealize, we delete the list,
destroy the damage and remove the callback (where the crash occurs):

  https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland.c#n577

The pixmap in question comes from GetWindowPixmap():

  https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland.c#n637

In our case, _fbGetWindowPixmap():

(gdb) p xwl_screen->screen->GetWindowPixmap
$2 = (GetWindowPixmapProcPtr) 0x4520d0 <_fbGetWindowPixmap>

Considering that the corresponding SetWindowPixmap
is damageSetWindowPixmap... I have no idea how we can end up with a pixmap
of size 0×0...

Cheers,
Olivier
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180123/61352ec8/attachment-0001.html>