Ensure no loops in toplevel parents#4894
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent invalid/cyclic xdg_toplevel parent relationships in the Wayland stable xdg-shell implementation, by rejecting self-parenting and parent/child loops.
Changes:
- Add a self-parent check in
XdgToplevelStable::set_parent()and raisexdg_toplevel::error.invalid_parent. - Add an ancestor-walk check intended to reject choosing a parent that is a descendant of the child toplevel.
| // Check that the parent is not a descendant of this toplevel | ||
| auto const this_surface = scene_surface(); | ||
| if (this_surface) | ||
| { | ||
| auto parent_surface = parent_toplevel->scene_surface(); | ||
| while (parent_surface) | ||
| { | ||
| if (parent_surface.value() == this_surface.value()) | ||
| { | ||
| BOOST_THROW_EXCEPTION(mw::ProtocolError( | ||
| resource, | ||
| Error::invalid_parent, | ||
| "Parent toplevel must not be a descendant of the child toplevel")); | ||
| } | ||
| auto const grandparent = parent_surface.value()->parent(); | ||
| parent_surface = grandparent ? std::optional{grandparent} : std::nullopt; | ||
| } |
There was a problem hiding this comment.
The cycle check walks parent_surface->parent() on mir::scene::Surface, but BasicSurface::parent() is backed by a parent_ member that is fixed at construction (so it won’t reflect later xdg_toplevel.set_parent reparenting handled in miral). This means a client can still create a cyclic toplevel-parent relationship that won’t be detected here, and will instead trip miral’s cycle detection (which throws std::runtime_error), causing wl_surface commit to hit the generic exception path (internal error) rather than a clean xdg_toplevel::error.invalid_parent. Consider validating against the same parent chain used by miral (WindowInfo parent links) or explicitly tracking the xdg-toplevel parent relationship (including pending/uncommitted changes) within XdgToplevelStable and using that for loop detection, so the protocol error is reliably raised here.
There was a problem hiding this comment.
Unfortunately, Copilot may be correct in this case. The parent set by WindowWlSurfaceRole::set_parent is const. I see no immediate way the parent we set and the parents we check connect.
| // Check that the parent is not a descendant of this toplevel | ||
| auto const this_surface = scene_surface(); | ||
| if (this_surface) | ||
| { | ||
| auto parent_surface = parent_toplevel->scene_surface(); | ||
| while (parent_surface) | ||
| { | ||
| if (parent_surface.value() == this_surface.value()) | ||
| { | ||
| BOOST_THROW_EXCEPTION(mw::ProtocolError( | ||
| resource, | ||
| Error::invalid_parent, | ||
| "Parent toplevel must not be a descendant of the child toplevel")); | ||
| } | ||
| auto const grandparent = parent_surface.value()->parent(); | ||
| parent_surface = grandparent ? std::optional{grandparent} : std::nullopt; | ||
| } |
There was a problem hiding this comment.
Unfortunately, Copilot may be correct in this case. The parent set by WindowWlSurfaceRole::set_parent is const. I see no immediate way the parent we set and the parents we check connect.
No description provided.