GNOME Bugzilla – Bug 783502
Lockup when using wl_shell_surface::set_transient on same surface
Last modified: 2017-06-22 06:51:05 UTC
Created attachment 353301 [details] Minimal example to trigger lock-up mutter locks up when using wl_shell_surface::set_transient() to set a surface transient to itself. I realize that this does not make sense, but still it should not crash the whole desktop. Minimal test case attached for easy reproduction. $ gcc `pkg-config --libs --cflags wayland-client` gsh_crash.c $ ./a.out (best run it in mutter --nested) This is the backtrace of the process when locked up for reference (debug symbols are missing, but I think you get the idea):
+ Trace 237557
Created attachment 353318 [details] [review] wl-shell: Refuse to make a surface transient to itself While this doesn't make sense, we should still deal with it to avoid an infinite loop when iterating over a window's transients.
Review of attachment 353318 [details] [review]: Looks fine. We have a similar kind of check in window.c (check_transient_for_loop()). I wonder if that is enough, or if we actually should check for loops here too. I think we have the same problem in meta-wayland-xdg-shell.c because I see no checking like this there either.
Can confirm that this fixes the problem for me. I tried to create a recursion with two surfaces by making them transient to each other, but that was already correctly ignored by some other code, which is good.
(In reply to Jonas Ådahl from comment #2) > Looks fine. We have a similar kind of check in window.c > (check_transient_for_loop()). I wonder if that is enough, or if we actually > should check for loops here too. That sounds like a good idea, yes. > I think we have the same problem in meta-wayland-xdg-shell.c because I > see no checking like this there either. Do you mean in xdg_toplevel_set_parent()? Any error there should be caught by the check in meta_window_set_transient_for() you pointed out. Or are you thinking about the parent_surface of popups?
(In reply to Florian Müllner from comment #4) > (In reply to Jonas Ådahl from comment #2) > > Looks fine. We have a similar kind of check in window.c > > (check_transient_for_loop()). I wonder if that is enough, or if we actually > > should check for loops here too. > > That sounds like a good idea, yes. > > > > I think we have the same problem in meta-wayland-xdg-shell.c because I > > see no checking like this there either. > > Do you mean in xdg_toplevel_set_parent()? Any error there should be caught > by the check in meta_window_set_transient_for() you pointed out. Or are you > thinking about the parent_surface of popups? Will it? Doesn't that function only go up the chains, without checking for window == parent? get_popup() might have same issue too maybe, I'd have to test to know, because that state tracking is a bit more complicated.
Created attachment 354193 [details] [review] window: Include window itself in transient loop check While it doesn't make sense to set a window as transient to itself, out existing check whether making a window transient doesn't cover it, so it's still possible to create an infinite loop. (In reply to Jonas Ådahl from comment #5) > Will it? Doesn't that function only go up the chains, without checking for > window == parent? You are right of course. Actually on second thought, extending that check should be enough to address the issue without duplicating it at all in the window backends ...
Review of attachment 354193 [details] [review]: lgtm.
Attachment 354193 [details] pushed as 5f49bda - window: Include window itself in transient loop check