After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759499 - wayland: DOS if an application set up as loop in transients
wayland: DOS if an application set up as loop in transients
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks: WaylandRelated
 
 
Reported: 2015-12-15 15:55 UTC by Olivier Fourdan
Modified: 2016-02-18 08:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Reproducer programm to trigger the issue (1.01 KB, text/plain)
2015-12-15 15:55 UTC, Olivier Fourdan
  Details
window: check for possible loop in transients (1.92 KB, patch)
2015-12-15 16:01 UTC, Olivier Fourdan
none Details | Review
Updated patch (1.72 KB, patch)
2015-12-16 08:03 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2015-12-15 15:55:42 UTC
Created attachment 317430 [details]
Reproducer programm to trigger the issue

Summary:

If a broken or naughty application set up its windows to create a loop in the transient relationship, mutter will hang, looping forever in meta_window_foreach_ancestor()

Steps to reproduce:

1. Log in GNOME on Wayland session (haven't tried in X11)
2. Save and build the attached reproducer:

   $ gcc -o cycling-transient-mutter -g cycling-transient-mutter.c `pkg-config --libs --cflags gtk+-3.0`

3. Run the program

   ./cycling-transient-mutter

Actual result:

gnome-shell will become unresponsive and take 100% CPU

Expected result:

gnome-shell survives a broken application.
Comment 1 Olivier Fourdan 2015-12-15 16:01:53 UTC
Created attachment 317431 [details] [review]
window: check for possible loop in transients

If a broken or naughty application tries set up its windows to create a loop in the transient relationship, mutter will hang, looping forever in meta_window_foreach_ancestor()
    
To avoid this, set the transient_for first to walk up the tree and detect all possibilities of a loop induced by this new transient relationship and deny the change if such a loop is found.
Comment 2 Jonas Ådahl 2015-12-16 04:08:51 UTC
Review of attachment 317431 [details] [review]:

::: src/core/window.c
@@ +7397,3 @@
+
+  previous = window->transient_for;
+  window->transient_for = parent;

This actually sets the field, which is done slightly different further below in meta_window_set_transient_for.

I think a check_ function should not set any field anyway.
Comment 3 Jonas Ådahl 2015-12-16 04:08:53 UTC
Review of attachment 317431 [details] [review]:

::: src/core/window.c
@@ +7397,3 @@
+
+  previous = window->transient_for;
+  window->transient_for = parent;

This actually sets the field, which is done slightly different further below in meta_window_set_transient_for.

I think a check_ function should not set any field anyway.
Comment 4 Olivier Fourdan 2015-12-16 08:03:02 UTC
Created attachment 317477 [details] [review]
Updated patch

Right, we don;t even need to set the transient for anyway, much simpler/safer patch attached.
Comment 5 Matthias Clasen 2016-02-18 05:18:44 UTC
This might still be useful to land for 3.20. Can it get reviewed ?
Comment 6 Jonas Ådahl 2016-02-18 05:30:54 UTC
Review of attachment 317477 [details] [review]:

Looks good to me. I suppose in the future we should, at least for Wayland clients, terminate abusing clients.
Comment 7 Olivier Fourdan 2016-02-18 08:07:08 UTC
Comment on attachment 317477 [details] [review]
Updated patch

attachment 317477 [details] [review] pushed as commit 4e82a75 window: check for possible loop in transients