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

[Daniel Stone]

Hi,

On 24 January 2018 at 16:17, Adam Jackson  wrote:
> On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:
>> So basically, just remove the  “if
>> (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?
>
> Worth a try anyway. I'm still just guessing at the root cause.

If it is the root cause, it certainly checks out:
  - window was realized, redirected, damaged
  - manual redirect removed (e.g. WM crashes or stops wanting to
present the window), pixmap destroyed (including damage)
  - possible: window is unrealized, but the DamageNotEmpty() check
doesn't pass due to the region being uninit'ed (or, if the window
lives, it's not unrealized)
  - block handler called
  - oops

Deleting the check in unrealize definitely makes sense, but perhaps to
guard against this even happening in the first place, xwl could just
place a manual redirect of its own when the window is realized and
removed when unrealized?

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Olivier Fourdan]

On Wed, Jan 24, 2018 at 5:17 PM, Adam Jackson  wrote:

> On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:
>
> > So basically, just remove the  “if
> > (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?
>
> Worth a try anyway. I'm still just guessing at the root cause.
>

Right, problem is I have no idea how to reproduce the issue so I cannot
tell if that would fix it.

But I don't think removing it unconditionally would cause any trouble (and
seems like the right thing to do), so... patch to follow.

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:

> So basically, just remove the  “if
> (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?

Worth a try anyway. I'm still just guessing at the root cause.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Olivier Fourdan]

Hi Adam,

On Tue, Jan 23, 2018 at 6:41 PM, Adam Jackson  wrote:

>
> Map / draw / unmap without hitting BlockHandler? I think
> xwl_unrealize_window() might be broken for that case:
>
> /* ... */
> wl_surface_destroy(xwl_window->surface);
> if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
> xorg_list_del(_window->link_damage);
> DamageUnregister(xwl_window->damage);
> DamageDestroy(xwl_window->damage);
> /* ... */
>
> If (for whatever reason) the damage region wasn't empty, we'd never
>

You mean “was empty” here, right, or do I misunderstand?

(we unlink if RegionNotEmpty() so we don't unlink if the region is empty)


> unlink this window from the dirty list. Should probably just unlink it
> unconditionally. If this is indeed what's happening, then the window
> being updated in xwl_window_post_damage() would have ->mapped = 0, and
> would be not the root window itself.
>

So basically, just remove the  “if
(RegionNotEmpty(DamageRegion(xwl_window->damage)))”
would suffice?

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

On Tue, 2018-01-23 at 10:15 +, Daniel Stone wrote:

> Ooh. serialNumber == 1 means it's the root pixmap, which will actually
> be uselessly empty. It would be interesting to see how we've ended up
> here: it would have to be a top-level window which a) was manually
> redirected by the WM when it was created, b) had damage posted on it,
> and c) was unredirected (in that order). I can't think of how that
> would happen;

Map / draw / unmap without hitting BlockHandler? I think
xwl_unrealize_window() might be broken for that case:

/* ... */
wl_surface_destroy(xwl_window->surface);
if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
xorg_list_del(_window->link_damage);
DamageUnregister(xwl_window->damage);
DamageDestroy(xwl_window->damage);
/* ... */

If (for whatever reason) the damage region wasn't empty, we'd never
unlink this window from the dirty list. Should probably just unlink it
unconditionally. If this is indeed what's happening, then the window
being updated in xwl_window_post_damage() would have ->mapped = 0, and
would be not the root window itself.

- ajax 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Olivier Fourdan]

Hi Daniel,

On Tue, Jan 23, 2018 at 11:15 AM, Daniel Stone  wrote:

> Ooh. serialNumber == 1 means it's the root pixmap, which will actually
> be uselessly empty. It would be interesting to see how we've ended up
> here: it would have to be a top-level window which a) was manually
> redirected by the WM when it was created, b) had damage posted on it,
> and c) was unredirected (in that order). I can't think of how that
> would happen; maybe you could place logs for the triggers (e.g.
> removing the last manual redirect on a window) somewhere?
>

Unfortunately I don't know how to reproduce that one, all I have is a
couple of core files...

But yes, that window is a toplevel window (hexchat):

Walking down the properties list until we get to the WM_CLASS, it gives
“hexchat”.

(gdb) x /s
xwl_window->window->optional->userProps->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->data
0x215bcf0:"hexchat"

with

(gdb) p
nodeTable[xwl_window->window->optional->userProps->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->propertyName]->string
$1 = 0x5b2c2c "WM_CLASS"

But the theory of the toplevel being created → redirected → damaged →
unredirected is plausible because I suspect this happens concurrently with
a crash in the Wayland compositor (gnome-shell) at startup while the apps
are restored via x-session management (from what I understood from the
issue).

(which, if my understanding is correct, means that the user session is
doomed anyway, so this crash in Xwayland might not be the culprit, but
still, I'd rather have Xwayland end with a “failed to read Wayland events”
rather than a segfault)

It would be good to see what the WindowRec under xwl_window looks
> like; maybe that could offer us a clue.
>

The xwl_window->window gives:

(gdb) p *xwl_window->window $2 = {drawable = {type = 0 '\000',
   class = 1 '\001',
   depth = 24 '\030',
   bitsPerPixel = 32 ' ',
   id = 27262979,
   x = 3407,
   y = 574,
   width = 665,
   height = 400,
   pScreen = 0x161d200,
   serialNumber = 11075},
   devPrivates = 0x215b998,
   parent = 0x1e5bff0,
   nextSib = 0x230c490,
   prevSib = 0x0,
   firstChild = 0x215bd60,
   lastChild = 0x215bd60,
   clipList = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0},
   data = 0x8137a0 },
   borderClip = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0},
 data = 0x8137a0 },
   valdata = 0x0,
   winSize = {extents = {x1 = 3407, y1 = 574, x2 = 4072, y2 = 974},
  data = 0x0},
   borderSize = {extents = {x1 = 3407, y1 = 574, x2 = 4072, y2 = 974},
 data = 0x0},
   origin = {x = 3407, y = 574},
   borderWidth = 0,
   deliverableEvents = 32895,
   eventMask = 6520959,
   background = {pixmap = 0xe8e8e7, pixel = 15263975},
   border = {pixmap = 0x0, pixel = 0},
   optional = 0x215bb10,
   backgroundState = 2,
   borderIsPixel = 1,
   cursorIsNone = 1,
   backingStore = 0,
   backStorage = 0,
   saveUnder = 0,
   bitGravity = 1,
   winGravity = 1,
   overrideRedirect = 0,
   visibility = 2, (VisibilityFullyObscured)
   mapped = 1,
   realized = 1,
   viewable = 1,
   dontPropagate = 0,
   forcedBS = 0,
   redirectDraw = 0,
   forcedBG = 0,
   unhittable = 0,
   damagedDescendants = 0,
   inhibitBGPaint = 0}

I didn't spot anything suspicious about it.

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Daniel Stone]

Hi,

On 23 January 2018 at 09:42, Olivier Fourdan  wrote:
> On 22 January 2018 at 19:57, Adam Jackson  wrote:
>> 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) f 7
> #7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@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():

Ooh. serialNumber == 1 means it's the root pixmap, which will actually
be uselessly empty. It would be interesting to see how we've ended up
here: it would have to be a top-level window which a) was manually
redirected by the WM when it was created, b) had damage posted on it,
and c) was unredirected (in that order). I can't think of how that
would happen; maybe you could place logs for the triggers (e.g.
removing the last manual redirect on a window) somewhere?

It would be good to see what the WindowRec under xwl_window looks
like; maybe that could offer us a clue.

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Olivier Fourdan]

Hey Adam,

On 22 January 2018 at 19:57, Adam Jackson  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  0x7f00239a31a7 in __GI_raise (sig=sig@entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f00239a4898 in __GI_abort () at abort.c:90
#2  0x0058f1da in OsAbort () at utils.c:1361
#3  0x00594ce3 in AbortServer () at log.c:877
#4  0x00595b2d in FatalError (f=f@entry=0x5b7490 "Caught signal %d
(%s). Server aborting\n") at log.c:1015
#5  0x0058c43c in OsSigHandler (signo=11, sip=,
unused=) at osinit.c:154
#6  
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@entry=0x1e5b6f0) at
xwayland-glamor.c:162
#8  0x00424da5 in xwl_screen_post_damage (xwl_screen=0x161d750) at
xwayland.c:514
#9  block_handler (data=0x161d750, timeout=) at
xwayland.c:665
#10 0x00557e46 in BlockHandler (pTimeout=pTimeout@entry=0x7ffeb514bf54)
at dixutils.c:388
#11 0x00585ed9 in WaitForSomething (are_ready=0) at WaitFor.c:219
#12 0x005531e1 in Dispatch () at dispatch.c:422
#13 0x0055744a in dix_main (argc=11, argv=0x7ffeb514c138,
envp=) at main.c:287
#14 0x7f002398f377 in __libc_start_main (main=0x4240d0 , argc=11,
ubp_av=0x7ffeb514c138, init=, fini=,
rtld_fini=, stack_end=0x7ffeb514c128) at
../csu/libc-start.c:274
#15 0x004240fe in _start ()

(gdb) f 7
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

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:
> 
>   #0  raise () from /usr/lib64/libc.so.6
>   #1  abort () from /usr/lib64/libc.so.6
>   #2  OsAbort () at utils.c:1361
>   #3  AbortServer () at log.c:877
>   #4  FatalError () at log.c:1015
>   #5  OsSigHandler () at osinit.c:154
>   #6  
>   #7  xwl_glamor_pixmap_get_wl_buffer () at xwayland-glamor.c:162
>   #8  xwl_screen_post_damage () at xwayland.c:514
>   #9  block_handler () at xwayland.c:665
>   #10 BlockHandler () at dixutils.c:388
>   #11 WaitForSomething () at WaitFor.c:219
>   #12 Dispatch () at dispatch.c:422
>   #13 dix_main () at main.c:287
> 
> 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.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Olivier Fourdan]

Hi Daniel,

On Thu, Jan 18, 2018 at 12:22 PM, Daniel Stone  wrote:

> Odd; how could we have a realized 0x0 window which also has damage? I
>

Hehe, yeap, I had the same question, but didn't find the answer... :)


> wonder if this isn't actually a UAF where the xwl_window has since
> been unrealized, in which case you should be able to reproduce pretty
> easily by causing damage on a window and then immediately destroying
> it. In that case, we just need
> wl_list_remove(_window->link_damage) inside
> xwl_window_unrealize().
>

But we do already do an “xorg_list_del(_window->link_damage);” in
xwl_window_unrealize()

However, we do that only if xwl_window is a thing and the damage region is
not empty:

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

Weird...

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Daniel Stone]

Hi Olivier,

On 18 January 2018 at 10:41, 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:
>
>   [...]
>   #6  
>   #7  xwl_glamor_pixmap_get_wl_buffer () at xwayland-glamor.c:162
>   #8  xwl_screen_post_damage () at xwayland.c:514
>   #9  block_handler () at xwayland.c:665
>   [...]
>
> 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.
>
> Avoid the NULL pointer dereference by checking the actual “xwl_pixmap”
> value in both glamor and shm implementations of pixmap_get_wl_buffer()
> and return a NULL buffer if there is no “xwl_pixmap”.

Odd; how could we have a realized 0x0 window which also has damage? I
wonder if this isn't actually a UAF where the xwl_window has since
been unrealized, in which case you should be able to reproduce pretty
easily by causing damage on a window and then immediately destroying
it. In that case, we just need
wl_list_remove(_window->link_damage) inside
xwl_window_unrealize().

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel