Quick heads-up, Xwayland present code causing memory corruption and crashes

[Olivier Fourdan]

Hi Roman,

Thanks for your reply!

On 18 April 2018 at 13:02, Roman Gilg <subdiff at gmail.com> wrote:

> On Wed, Apr 18, 2018 at 11:25 AM, Olivier Fourdan <ofourdan at redhat.com>
> wrote:
> > This is a quick heads up, I was experiencing random crashes in Xwayland
> > (very annoying in gnome-shell and it takes gnome-shell and the entire
> > session with it). while running skype with current Xwayladn from 1.20
> rc4.
>
> Sorry for the annoyances. But I'm of course very glad you found them
> before release. Thanks!
>
> > This is due to commit 82df2ce3:
> >
> > Author: Roman Gilg <subdiff at gmail.com>
> > Date:   Tue Aug 22 15:38:26 2017 +0200
> >
> >     xwayland: Avoid repeatedly looping through window ancestor chain
> >
> >     Calling xwl_window_from_window means looping through the window
> ancestor
> >     chain whenever it is called on a child window or on an automatically
> >     redirected window.
> >
> >     Since these properties and the potential ancestor's xwl_window are
> > constant
> >     between window realization and unrealization, we can omit the
> looping by
> >     always putting the respective xwl_window in the Window's private
> field
> > on
> >     its realization. If the Window doesn't feature an xwl_window on its
> own,
> >     it's the xwl_window of its first ancestor with one.
> >
> >     Signed-off-by: Roman Gilg <subdiff at gmail.com>
> >     Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > I think the assumption there is invalid in the case of a UnrealizeTree().
>
> In UnrealizeTree() it seems indeed the tree is unrealized top to
> bottom. What's the reason for doing it this way? Naively I would think
> that unrealizing a window tree should run in opposed direction to
> realizing one.
>

I don't know the reason for this.


> > Reverting that patch (and adjusting xwayland-present.c accordingly)
> solves
> > the “invalid read” problem (as we don;t point to freed memoery anymore),
> but
> > unfortunately that breaks xwayland-present logic that now leaves pending
> > buffers and events after the unrealize()...
>
> Reverting the patch and always calling xwl_window_from_window in the
> present code (of course this means that we loop quite often in the
> xwayland-present code) should work, since xwl_present_cleanup is
> called in xwl_unrealize_window also already for the top parent window.
>

Yeah, I have a patch ready for that, with some minor tweaks as well.


> To fix the logic in xwl_present_cleanup we would need comparison
> between event->present_window and xwl_window->present_window. And
> destroy the xwl_window->present_frame_callback also if cleanup is
> called on the top parent window.
>
> So should we do this or try to always unrealize bottom-to-top in
> UnrealizeTree()?


I suspect changing the logic in UnrealizeTree() might unvocer even more
problems, sp close to a release.

I'd rather fix that in Xwayland code instead. I'll post my patch so you can
elaborate on that if you will.

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