[PATCH v2] present: fix freed pointer access

[Lionel Landwerlin]

When a vblank has been marked as aborted, it's going to be free in the
flip_notify function when stopped. We can't notify it after it's
stopped because the pointer is invalid.

Valgrind backtrace:

==5331== Invalid read of size 8
==5331== at 0x212B4D: present_vblank_notify (present_vblank.c:34)
==5331== by 0x21439B: present_wnmd_flip_notify (present_wnmd.c:194)
==5331== by 0x21439B: present_wnmd_event_notify (present_wnmd.c:228)
==5331== by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
==5331== by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331== by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331== by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x14BCCA: xwl_read_events (xwayland.c:814)
==5331== by 0x2AC0D0: ospoll_wait (ospoll.c:651)
==5331== by 0x2A5322: WaitForSomething (WaitFor.c:208)
==5331== by 0x27574B: Dispatch (dispatch.c:421)
==5331== Address 0x1b44dc98 is 40 bytes inside a block of size 184 free'd
==5331== at 0x48369EB: free (vg_replace_malloc.c:530)
==5331== by 0x213B0A: present_wnmd_free_idle_vblanks (present_wnmd.c:118)
==5331== by 0x213B0A: present_wnmd_flips_stop (present_wnmd.c:161)
==5331== by 0x2143EF: present_wnmd_flip_notify (present_wnmd.c:192)
==5331== by 0x2143EF: present_wnmd_event_notify (present_wnmd.c:228)
==5331== by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
==5331== by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331== by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331== by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331== by 0x14BCCA: xwl_read_events (xwayland.c:814)
==5331== by 0x2AC0D0: ospoll_wait (ospoll.c:651)
==5331== by 0x2A5322: WaitForSomething (WaitFor.c:208)
==5331== Block was alloc'd at
==5331== at 0x48377D5: calloc (vg_replace_malloc.c:711)
==5331== by 0x212D9F: present_vblank_create (present_vblank.c:69)
==5331== by 0x214014: present_wnmd_pixmap (present_wnmd.c:610)
==5331== by 0x21576C: proc_present_pixmap (present_request.c:150)
==5331== by 0x27599D: Dispatch (dispatch.c:479)
==5331== by 0x279945: dix_main (main.c:276)
==5331== by 0x633AB16: (below main) (libc-start.c:310)

v2: Still notify aborted flips (Roman)

Signed-off-by: Lionel Landwerlin <***@intel.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
---
present/present_wnmd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 80ffb014e..a26a54f6a 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -188,10 +188,11 @@ present_wnmd_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_
window_priv->flip_active = vblank;
window_priv->flip_pending = NULL;

+ present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
+
if (vblank->abort_flip)
present_wnmd_flips_stop(window);

- present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
present_wnmd_flip_try_ready(window);
}

[Roman Gilg]

On Mon, Aug 27, 2018 at 2:06 AM Lionel Landwerlin <

Post by Lionel Landwerlin
v2: Still notify aborted flips (Roman)

I also want to test it later this week when I've finished some other work,
but for now:

Reviewed-by: Roman Gilg <***@gmail.com>

[Roman Gilg]

So I have it running now without problems for a few days. I tested it once
with CS:GO and Hitman and there were no crashes. I can't test the suspend
recovery because this is currently not working due to a problem in KWin.

Tested-by: Roman Gilg <***@gmail.com>

[Roman Gilg]

Ok, I just got a failing assert in xwl_present_flips_stop with the patch
when opening a context menu in Steam. Seems the xwl_present_flips_stop call
is coming in too late now after the presenting window has already been
changed.
#0 __GI_raise (sig=***@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007f9f5a5f1801 in __GI_abort () at abort.c:79
#2 0x0000564a52bda52a in OsAbort () at ../../src/xserver/os/utils.c:1350
#3 0x0000564a52bdf733 in AbortServer () at ../../src/xserver/os/log.c:877
#4 0x0000564a52be0555 in FatalError (f=***@entry=0x564a52c21c70 "Caught
signal %d (%s). Server aborting\n") at ../../src/xserver/os/log.c:1015
#5 0x0000564a52bd7613 in OsSigHandler (signo=6, sip=<optimized out>,
unused=<optimized out>) at ../../src/xserver/os/osinit.c:156
#6 <signal handler called>
#7 __GI_raise (sig=***@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#8 0x00007f9f5a5f1801 in __GI_abort () at abort.c:79
#9 0x00007f9f5a5e139a in __assert_fail_base (fmt=0x7f9f5a7687d8
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=***@entry=0x564a52c0d9e0
"xwl_window->present_window == window", file=***@entry=0x564a52c0d9a8
"../../src/xserver/hw/xwayland/xwayland-present.c", line=***@entry=516,
function=***@entry=0x564a52c0da20 <__PRETTY_FUNCTION__.25179>
"xwl_present_flips_stop") at assert.c:92
#10 0x00007f9f5a5e1412 in __GI___assert_fail
(assertion=***@entry=0x564a52c0d9e0
"xwl_window->present_window == window", file=***@entry=0x564a52c0d9a8
"../../src/xserver/hw/xwayland/xwayland-present.c", line=***@entry=516,
function=***@entry=0x564a52c0da20 <__PRETTY_FUNCTION__.25179>
"xwl_present_flips_stop") at assert.c:101
#11 0x0000564a52aa817b in xwl_present_flips_stop (window=0x564a544fda10) at
../../src/xserver/hw/xwayland/xwayland-present.c:516
#12 0x0000564a52b65968 in present_wnmd_flips_stop (window=<optimized out>)
at ../../src/xserver/present/present_wnmd.c:159
#13 0x0000564a52b65bc5 in present_wnmd_check_flip_window
(window=0x564a544fda10) at ../../src/xserver/present/present_wnmd.c:332
#14 0x0000564a52b642af in present_clip_notify (window=0x564a544fda10,
dx=896, dy=471) at ../../src/xserver/present/present_screen.c:203
#15 0x0000564a52b3a422 in compClipNotify (pWin=0x564a544fda10, dx=896,
dy=471) at ../../src/xserver/composite/compwindow.c:317
#16 0x0000564a52ae950a in miComputeClips (pParent=***@entry=0x564a544fda10,
pScreen=***@entry=0x564a53de3970, universe=***@entry=0x7fff351d9cb0,
kind=***@entry=VTOther, exposed=***@entry=0x7fff351d9e30) at
../../src/xserver/mi/mivaltree.c:478
#17 0x0000564a52ae9833 in miComputeClips (pParent=***@entry=0x564a54868030,
pScreen=***@entry=0x564a53de3970, universe=***@entry=0x7fff351d9d60,
kind=***@entry=VTOther, exposed=***@entry=0x7fff351d9e30) at
../../src/xserver/mi/mivaltree.c:428
#18 0x0000564a52ae9833 in miComputeClips (pParent=***@entry=0x564a54867ea0,
pScreen=***@entry=0x564a53de3970, universe=***@entry=0x7fff351d9e10,
kind=***@entry=VTOther, exposed=***@entry=0x7fff351d9e30) at
../../src/xserver/mi/mivaltree.c:428
#19 0x0000564a52ae9ab3 in miValidateTree (pParent=0x564a53fb0570,
pChild=0x564a54867ea0, kind=<optimized out>) at
../../src/xserver/mi/mivaltree.c:681
#20 0x0000564a52af08a1 in miResizeWindow (pWin=0x564a54867ea0, x=896,
y=471, w=<optimized out>, h=<optimized out>, pSib=0x0) at
../../src/xserver/mi/miwindow.c:467
#21 0x0000564a52b3aaaa in compResizeWindow (pWin=0x564a54867ea0,
x=<optimized out>, y=<optimized out>, w=<optimized out>, h=<optimized
out>, pSib=<optimized out>) at ../../src/xserver/composite/compwindow.c:407
#22 0x0000564a52b31144 in ConfigureWindow (pWin=<optimized out>,
mask=<optimized out>, vlist=***@entry=0x564a5420c620,
client=***@entry=0x564a542020c0)
at ../../src/xserver/dix/window.c:2422
#23 0x0000564a52b00469 in ProcConfigureWindow (client=0x564a542020c0) at
../../src/xserver/dix/dispatch.c:916
#24 0x0000564a52b06178 in Dispatch () at
../../src/xserver/dix/dispatch.c:478
#25 0x0000564a52b0a178 in dix_main (argc=6, argv=0x7fff351da2a8,
envp=<optimized out>) at ../../src/xserver/dix/main.c:276
#26 0x00007f9f5a5d2b97 in __libc_start_main (main=0x564a52a9cf30 <main>,
argc=6, argv=0x7fff351da2a8, init=<optimized out>, fini=<optimized out>,
rtld_fini=<optimized out>, stack_end=0x7fff351da298) at
../csu/libc-start.c:310
#27 0x0000564a52a9cf6a in _start ()

[Lionel Landwerlin]

Oh well...
I'm sure you'll be able to fix it faster than me :)

-
Lionel

Post by Roman Gilg
Ok, I just got a failing assert in xwl_present_flips_stop with the
patch when opening a context menu in Steam. Seems the
xwl_present_flips_stop call is coming in too late now after the
presenting window has already been changed.
#1  0x00007f9f5a5f1801 in __GI_abort () at abort.c:79
#2  0x0000564a52bda52a in OsAbort () at ../../src/xserver/os/utils.c:1350
#3  0x0000564a52bdf733 in AbortServer () at ../../src/xserver/os/log.c:877
signal %d (%s). Server aborting\n") at ../../src/xserver/os/log.c:1015
#5  0x0000564a52bd7613 in OsSigHandler (signo=6, sip=<optimized out>,
unused=<optimized out>) at ../../src/xserver/os/osinit.c:156
#6  <signal handler called>
#8  0x00007f9f5a5f1801 in __GI_abort () at abort.c:79
#9  0x00007f9f5a5e139a in __assert_fail_base (fmt=0x7f9f5a7687d8
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
"../../src/xserver/hw/xwayland/xwayland-present.c",
<__PRETTY_FUNCTION__.25179> "xwl_present_flips_stop") at assert.c:92
#10 0x00007f9f5a5e1412 in __GI___assert_fail
"../../src/xserver/hw/xwayland/xwayland-present.c",
<__PRETTY_FUNCTION__.25179> "xwl_present_flips_stop") at assert.c:101
#11 0x0000564a52aa817b in xwl_present_flips_stop
(window=0x564a544fda10) at
../../src/xserver/hw/xwayland/xwayland-present.c:516
#12 0x0000564a52b65968 in present_wnmd_flips_stop (window=<optimized
out>) at ../../src/xserver/present/present_wnmd.c:159
#13 0x0000564a52b65bc5 in present_wnmd_check_flip_window
(window=0x564a544fda10) at ../../src/xserver/present/present_wnmd.c:332
#14 0x0000564a52b642af in present_clip_notify (window=0x564a544fda10,
dx=896, dy=471) at ../../src/xserver/present/present_screen.c:203
#15 0x0000564a52b3a422 in compClipNotify (pWin=0x564a544fda10, dx=896,
dy=471) at ../../src/xserver/composite/compwindow.c:317
#16 0x0000564a52ae950a in miComputeClips
../../src/xserver/mi/mivaltree.c:478
#17 0x0000564a52ae9833 in miComputeClips
../../src/xserver/mi/mivaltree.c:428
#18 0x0000564a52ae9833 in miComputeClips
../../src/xserver/mi/mivaltree.c:428
#19 0x0000564a52ae9ab3 in miValidateTree (pParent=0x564a53fb0570,
pChild=0x564a54867ea0, kind=<optimized out>) at
../../src/xserver/mi/mivaltree.c:681
#20 0x0000564a52af08a1 in miResizeWindow (pWin=0x564a54867ea0, x=896,
y=471, w=<optimized out>, h=<optimized out>, pSib=0x0) at
../../src/xserver/mi/miwindow.c:467
#21 0x0000564a52b3aaaa in compResizeWindow (pWin=0x564a54867ea0,
x=<optimized out>, y=<optimized out>, w=<optimized    out>,
h=<optimized out>, pSib=<optimized out>) at
../../src/xserver/composite/compwindow.c:407
#22 0x0000564a52b31144 in ConfigureWindow (pWin=<optimized out>,
#23 0x0000564a52b00469 in ProcConfigureWindow (client=0x564a542020c0)
at ../../src/xserver/dix/dispatch.c:916
#24 0x0000564a52b06178 in Dispatch () at
../../src/xserver/dix/dispatch.c:478
#25 0x0000564a52b0a178 in dix_main (argc=6, argv=0x7fff351da2a8,
envp=<optimized out>) at ../../src/xserver/dix/main.c:276
#26 0x00007f9f5a5d2b97 in __libc_start_main (main=0x564a52a9cf30
<main>, argc=6, argv=0x7fff351da2a8, init=<optimized out>,
fini=<optimized out>, rtld_fini=<optimized out>,
stack_end=0x7fff351da298) at ../csu/libc-start.c:310
#27 0x0000564a52a9cf6a in _start ()
_______________________________________________
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[Olivier Fourdan]

Hi,

On Tue, Sep 4, 2018 at 6:28 PM Lionel Landwerlin

Post by Lionel Landwerlin
Oh well...
I'm sure you'll be able to fix it faster than me :)
-
Lionel
Ok, I just got a failing assert in xwl_present_flips_stop with the patch when opening a context menu in Steam. Seems the xwl_present_flips_stop call is coming in too late now after the presenting window has already been changed.

[...]

If I read bug 107314 correctly, the crash occurs after the window has
been destroyed, so what about that other patch:

https://patchwork.freedesktop.org/patch/247271/

** plus ** this patch below (just copied for testing purpose), does it
fix th your crash?

From 676597fcd6ee907f4d3f165dd0b5de746f7c8131 Mon Sep 17 00:00:00 2001
From: Olivier Fourdan <***@redhat.com>
Date: Wed, 5 Sep 2018 13:08:03 +0200
Subject: [PATCH xserver] xwayland: ignore sync callback if window is destroyed

If the window is destroyed, there is no need to send the vblank notify
event.

This should avoid a crash in present_vblank_notify()

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
Signed-off-by: Olivier Fourdan <***@redhat.com>
---
hw/xwayland/xwayland-present.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 316e04443..b1751c846 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -276,6 +276,10 @@ xwl_present_sync_callback(void *data,

event->pending = FALSE;

+ /* Is the window destroyed already ? */
+ if (!xwl_present_window)
+ return;
+
if (event->abort) {
/* Event might have been aborted */
if (event->buffer_released)

[Olivier Fourdan]

Hi,

Post by Olivier Fourdan
Hi,
On Tue, Sep 4, 2018 at 6:28 PM Lionel Landwerlin

Post by Lionel Landwerlin
Oh well...
I'm sure you'll be able to fix it faster than me :)
-
Lionel

[...]
If I read bug 107314 correctly, the crash occurs after the window has
https://patchwork.freedesktop.org/patch/247271/
** plus ** this patch below (just copied for testing purpose), does it
fix your crash?

Sorry, that's unrelated, please ignore my last post...

So I agree with Lionel's v2 patch
(https://patchwork.freedesktop.org/patch/246007/) to fix the issue
with the use-after-free.

Post by Olivier Fourdan

Post by Lionel Landwerlin
Ok, I just got a failing assert in xwl_present_flips_stop with the patch when opening a context menu in Steam.
Seems the xwl_present_flips_stop call is coming in too late now after the presenting window has already been
changed.

I fail to understand how that can be related to Lionel's patch though.

The patch simply moves `present_vblank_notify()` before
`present_wnmd_flips_stop()` in `present_wnmd_flip_notify()` whereas
the assertion failure comes from an eniterly different code path, down
from `present_clip_notify()` which calls
`present_wnmd_check_flip_window()` which does:

```
325 if (flip_pending) {
326 if (!present_wnmd_check_flip(flip_pending->crtc,
flip_pending->window, flip_pending->pixmap,
327 flip_pending->sync_flip, NULL, 0, 0, NULL))
328 present_wnmd_set_abort_flip(window);
329 } else if (flip_active) {
330 if (!present_wnmd_check_flip(flip_active->crtc,
flip_active->window, flip_active->pixmap,
331 flip_active->sync_flip, NULL,
0, 0, NULL))
332 present_wnmd_flips_stop(window);
333 }
```

`present_wnmd_check_flip()` calls `xwl_present_check_flip2()` which does:

```
424 * Do not flip if there is already another child window doing flips.
425 */
426 if (xwl_window->present_window &&
427 xwl_window->present_window != present_window)
428 return FALSE;
429
```

So here we end up with `assert(xwl_window->present_window == window)`
in `xwl_present_flips_stop()` when `present_wnmd_check_flip()` returns
FALSE, i.e. when `xwl_window->present_window != present_window`.

I'm confused :/

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

[Olivier Fourdan]

() retHi,

Post by Olivier Fourdan

Post by Lionel Landwerlin
Ok, I just got a failing assert in xwl_present_flips_stop with the patch when opening a context menu in Steam.
Seems the xwl_present_flips_stop call is coming in too late now after the presenting window has already been
changed.

I fail to understand how that can be related to Lionel's patch though.
The patch simply moves `present_vblank_notify()` before
`present_wnmd_flips_stop()` in `present_wnmd_flip_notify()` whereas
the assertion failure comes from an eniterly different code path, down
from `present_clip_notify()` which calls
[...]

I've hit that assertion in xwl_present_flips_stop() as well, but the
backtrace is slightly different, it happened during a
XReparentWindow().

In order to make that email short enough, I've attached how I ended up
to the conclusion that there is a perfectly valid code path that can
lead to xwl_present_flips_stop() with xwl_window->present_window being
NULL and that will cause a crash.

Cheers,
Olivier