[PATCH v2] present: fix freed pointer access
Olivier Fourdan
ofourdan at redhat.com
Thu Sep 13 16:49:15 UTC 2018
() retHi,
On Fri, Sep 7, 2018 at 1:16 PM Olivier Fourdan <ofourdan at redhat.com> wrote:
>
> > > On 04/09/2018 16:27, Roman Gilg wrote:
> > >
> > > 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:
> [...]
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
-------------- next part --------------
(gdb) bt
#0 __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f2182f24895 in __GI_abort () at abort.c:79
#2 0x00000000005943f0 in OsAbort () at utils.c:1350
#3 0x0000000000599689 in AbortServer () at log.c:877
#4 0x000000000059a4fd in FatalError (
f=f at entry=0x5c0770 "Caught signal %d (%s). Server aborting\n")
at log.c:1015
#5 0x00000000005916f5 in OsSigHandler (signo=6, sip=<optimized out>,
unused=<optimized out>) at osinit.c:156
#6 <signal handler called>
#7 __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#8 0x00007f2182f24895 in __GI_abort () at abort.c:79
#9 0x00007f2182f24769 in __assert_fail_base (
fmt=0x7f218308bea8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=0x5a6c18 "xwl_window->present_window == window",
file=0x5a6c03 "xwayland-present.c", line=521, function=<optimized out>)
at assert.c:92
#10 0x00007f2182f329f6 in __GI___assert_fail (
assertion=assertion at entry=0x5a6c18 "xwl_window->present_window == window",
file=file at entry=0x5a6c03 "xwayland-present.c", line=line at entry=521,
function=function at entry=0x5a6c40 <__PRETTY_FUNCTION__.25116> "xwl_present_flips_stop") at assert.c:101
#11 0x00000000004395d5 in xwl_present_flips_stop (window=0x21be120)
at xwayland-present.c:521
#12 0x00000000004f8be8 in present_wnmd_flips_stop (window=<optimized out>)
at present_wnmd.c:159
#13 0x00000000004f8e55 in present_wnmd_check_flip_window (window=0x21be120)
at present_wnmd.c:332
#14 0x00000000004f73f3 in present_clip_notify (window=0x21be120, dx=0, dy=0)
at present_screen.c:203
#15 0x0000000000547d4f in compClipNotify (pWin=0x21be120, dx=0, dy=0)
at compwindow.c:317
#16 0x0000000000480b2b in miComputeClips (pParent=pParent at entry=0x21be120,
pScreen=pScreen at entry=0xbbbf00, universe=universe at entry=0x7ffd04c4d1d0,
kind=kind at entry=VTMap, exposed=exposed at entry=0x7ffd04c4d1f0)
at mivaltree.c:478
#17 0x0000000000481057 in miValidateTree (pParent=0x12c5ed0, pChild=0x21be120,
kind=<optimized out>) at mivaltree.c:681
#18 0x000000000058778a in MapWindow (pWin=0x21be120, client=0x1629a30)
at window.c:2699
#19 0x000000000058ab2c in ReparentWindow (pWin=0x21be120, pParent=0x12c5ed0,
x=<optimized out>, y=<optimized out>, client=client at entry=0x1629a30)
at window.c:2600
#20 0x000000000055559c in ProcReparentWindow (client=0x1629a30)
at dispatch.c:829
#21 0x000000000055b78e in Dispatch () at dispatch.c:478
#22 0x000000000055f7d6 in dix_main (argc=12, argv=0x7ffd04c4d4f8,
envp=<optimized out>) at main.c:276
#23 0x00007f2182f26413 in __libc_start_main (main=0x42e300 <main>, argc=12,
argv=0x7ffd04c4d4f8, init=<optimized out>, fini=<optimized out>,
rtld_fini=<optimized out>, stack_end=0x7ffd04c4d4e8)
at ../csu/libc-start.c:308
#24 0x000000000042e33e in _start ()
Unfortunately, the `xwl_window` has been optimized out, so I can't get to the value of `xwl_window->present_window` in `xwl_present_flips_stop()`:
(gdb) f 11
#11 0x00000000004395d5 in xwl_present_flips_stop (window=0x21be120)
at xwayland-present.c:521
521 assert(xwl_window->present_window == window);
(gdb) p xwl_window->present_window
value has been optimized out
Fortunately, we can recompute its value:
512 static void
513 xwl_present_flips_stop(WindowPtr window)
514 {
515 struct xwl_window *xwl_window = xwl_window_from_window(window);
516 struct xwl_present_window *xwl_present_window = xwl_present_window_priv(window);
With xwl_window_from_window() being:
259 xwl_window_from_window(WindowPtr window)
260 {
261 struct xwl_window *xwl_window;
262
263 while (window) {
264 xwl_window = xwl_window_get(window);
265 if (xwl_window)
266 return xwl_window;
267
268 window = window->parent;
269 }
270
271 return NULL;
272 }
And xwl_window_get():
129 static struct xwl_window *
130 xwl_window_get(WindowPtr window)
131 {
132 return dixLookupPrivate(&window->devPrivates, &xwl_window_private_key);
133 }
So back to the code file:
(gdb) f 13
#13 0x00000000004f8e55 in present_wnmd_check_flip_window (window=0x21be120) at present_wnmd.c:332
332 present_wnmd_flips_stop(window);
(gdb) p *(struct xwl_window *) ((char *) window->devPrivates + xwl_window_private_key->offset)
$186 = {xwl_screen = 0x1a6cb70, surface = 0x0, shell_surface = 0x1a22450, window = 0x21c1ed0, damage = 0x21a1350,
link_damage = {next = 0x0, prev = 0x216bf60}, frame_callback = 0x0, allow_commits = 35377760, present_window = 0x0}
(gdb)
So xwl_window->present_window is NULL so obviously xwl_window->present_window == window is FALSE and the assert() is triggers an abort().
Right, but how can we end up there?
Let's got back a couple of steps higher in the backtrace, in present_wnmd_check_flip_window():
304 * 'window' is being reconfigured. Check to see if it is involved
305 * in flipping and clean up as necessary.
306 */
307 static void
308 present_wnmd_check_flip_window (WindowPtr window)
309 {
...
322 flip_pending = window_priv->flip_pending;
323 flip_active = window_priv->flip_active;
324
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 }
If we get to present_wnmd_flips_stop(window) it means that present_wnmd_check_flip() returned FALSE.
present_wnmd_check_flip() reads:
243 static Bool
244 present_wnmd_check_flip(RRCrtcPtr crtc,
245 WindowPtr window,
246 PixmapPtr pixmap,
247 Bool sync_flip,
248 RegionPtr valid,
249 int16_t x_off,
250 int16_t y_off,
251 PresentFlipReason *reason)
252 {
253 ScreenPtr screen = window->drawable.pScreen;
254 present_screen_priv_ptr screen_priv = present_screen_priv(screen);
255 WindowPtr toplvl_window = present_wnmd_toplvl_pixmap_window(window);
256
257 if (reason)
258 *reason = PRESENT_FLIP_REASON_UNKNOWN;
259
260 if (!screen_priv)
261 return FALSE;
262
263 if (!screen_priv->wnmd_info)
264 return FALSE;
265
266 if (!crtc)
267 return FALSE;
268
269 /* Check to see if the driver supports flips at all */
270 if (!screen_priv->wnmd_info->flip)
271 return FALSE;
272
273 /* Don't flip redirected windows */
274 if (window->redirectDraw != RedirectDrawNone)
275 return FALSE;
276
277 /* Source pixmap must align with window exactly */
278 if (x_off || y_off)
279 return FALSE;
280
281 // TODO: Check for valid region?
282
283 /* Flip pixmap must have same dimensions as window */
284 if (window->drawable.width != pixmap->drawable.width ||
285 window->drawable.height != pixmap->drawable.height)
286 return FALSE;
287
288 /* Window must be same region as toplevel window */
289 if ( !RegionEqual(&window->winSize, &toplvl_window->winSize) )
290 return FALSE;
291
292 /* Ask the driver for permission */
293 if (screen_priv->wnmd_info->check_flip2) {
294 if (!(*screen_priv->wnmd_info->check_flip2) (crtc, window, pixmap, sync_flip, reason)) {
295 DebugPresent(("\td %08lx -> %08lx\n", window->drawable.id, pixmap ? pixmap->drawable.id : 0));
296 return FALSE;
297 }
298 }
299
300 return TRUE;
301 }
Again, back to the core file:
(gdb) f 13
#13 0x00000000004f8e55 in present_wnmd_check_flip_window (window=0x21be120)
at present_wnmd.c:332
332 present_wnmd_flips_stop(window);
(gdb) p *window_priv->flip_active
$1 = {window_list = {next = 0x21bd910, prev = 0x21bd910}, event_queue = {
next = 0x21bd920, prev = 0x21bd920}, screen = 0xbbbf00,
window = 0x21be120, pixmap = 0x21d8f20, valid = 0x0, update = 0x0,
crtc = 0xbce6c0, serial = 1, x_off = 0, y_off = 0, kind = 0, event_id = 1,
target_msc = 1, msc_offset = 0, idle_fence = 0x21f1760, wait_fence = 0x0,
notifies = 0x0, num_notifies = 0, queued = 0, requeue = 0, flip = 1,
flip_ready = 0, flip_idler = 0, sync_flip = 0, abort_flip = 0,
reason = PRESENT_FLIP_REASON_UNKNOWN, has_suboptimal = 8}
I checked, screen_priv is valid, so is screen_priv->wnmd_info, etc. *but* window->redirectDraw == 2 so window->redirectDraw != RedirectDrawNone and we return FALSE here...
Fnny thing is, if we didn't return FALSE early on here, we we would have get to xwl_present_check_flip2() which does check for xwl_window->present_window being non-NULL:
408 xwl_present_check_flip2(RRCrtcPtr crtc,
409 WindowPtr present_window,
410 PixmapPtr pixmap,
411 Bool sync_flip,
412 PresentFlipReason *reason)
413 {
414 struct xwl_window *xwl_window = xwl_window_from_window(present_window);
415
416 if (!xwl_window)
417 return FALSE;
418
419 /*
420 * Do not flip if there is already another child window doing flips.
421 */
422 if (xwl_window->present_window &&
423 xwl_window->present_window != present_window)
424 return FALSE;
So I reckon 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.
More information about the xorg-devel
mailing list