From 2f5337b6794813ad5151c3776595ac4967d0c76f Mon Sep 17 00:00:00 2001 From: Christian Meissl Date: Fri, 6 Feb 2026 18:45:59 +0100 Subject: [PATCH] compositor: prevent adding hooks and blockers on destroyed surfaces technically it shouldn't be an issue to add those to destroyed surfaces, but preventing it might make catching logic issues easier. --- anvil/src/shell/mod.rs | 6 ++-- anvil/src/shell/xdg.rs | 3 +- src/backend/renderer/utils/wayland.rs | 2 +- src/wayland/commit_timing/mod.rs | 4 +-- src/wayland/compositor/mod.rs | 34 +++++++++++++++++------ src/wayland/drm_syncobj/mod.rs | 12 ++++++-- src/wayland/fifo/mod.rs | 4 +-- src/wayland/pointer_constraints.rs | 2 +- src/wayland/seat/keyboard.rs | 23 +++++++++------ src/wayland/session_lock/lock.rs | 10 +++++-- src/wayland/shell/wlr_layer/handlers.rs | 2 +- src/wayland/shell/xdg/handlers/surface.rs | 4 +-- src/wayland/viewporter/mod.rs | 2 +- src/wayland/xwayland_shell.rs | 4 ++- 14 files changed, 74 insertions(+), 38 deletions(-) diff --git a/anvil/src/shell/mod.rs b/anvil/src/shell/mod.rs index 541383ff2376..984906b8bc41 100644 --- a/anvil/src/shell/mod.rs +++ b/anvil/src/shell/mod.rs @@ -115,7 +115,7 @@ impl CompositorHandler for AnvilState { } fn new_surface(&mut self, surface: &WlSurface) { - add_pre_commit_hook::(surface, move |state, _dh, surface| { + let _ = add_pre_commit_hook::(surface, move |state, _dh, surface| { #[cfg(feature = "udev")] let mut acquire_point = None; let maybe_dmabuf = with_states(surface, |surface_data| { @@ -149,7 +149,7 @@ impl CompositorHandler for AnvilState { Ok(()) }); if res.is_ok() { - add_blocker(surface, blocker); + let _ = add_blocker(surface, blocker); return; } } @@ -162,7 +162,7 @@ impl CompositorHandler for AnvilState { Ok(()) }); if res.is_ok() { - add_blocker(surface, blocker); + let _ = add_blocker(surface, blocker); } } } diff --git a/anvil/src/shell/xdg.rs b/anvil/src/shell/xdg.rs index 7b31b5af8b83..4fafff565df6 100644 --- a/anvil/src/shell/xdg.rs +++ b/anvil/src/shell/xdg.rs @@ -50,7 +50,8 @@ impl XdgShellHandler for AnvilState { let window = WindowElement(Window::new_wayland_window(surface.clone())); place_new_window(&mut self.space, self.pointer.current_location(), &window, true); - compositor::add_post_commit_hook(surface.wl_surface(), |state: &mut Self, _, surface| { + // FIXME: On role re-usage this might be problematic + let _ = compositor::add_post_commit_hook(surface.wl_surface(), |state: &mut Self, _, surface| { handle_toplevel_commit(&mut state.space, surface); }); } diff --git a/src/backend/renderer/utils/wayland.rs b/src/backend/renderer/utils/wayland.rs index bc8b0c70add9..66250346b27c 100644 --- a/src/backend/renderer/utils/wayland.rs +++ b/src/backend/renderer/utils/wayland.rs @@ -391,7 +391,7 @@ pub fn on_commit_buffer_handler(surface: &WlSurface) { |_, _, _| true, ); for surf in &new_surfaces { - add_destruction_hook(surf, |_: &mut D, surface| { + let _ = add_destruction_hook(surf, |_: &mut D, surface| { // We reset the state on destruction before the user_data is dropped // to prevent a deadlock which can happen if we try to send a buffer // release during drop. This also enables us to free resources earlier diff --git a/src/wayland/commit_timing/mod.rs b/src/wayland/commit_timing/mod.rs index aeba97dfd56b..c6c117047431 100644 --- a/src/wayland/commit_timing/mod.rs +++ b/src/wayland/commit_timing/mod.rs @@ -210,7 +210,7 @@ where // Make sure we do not install the hook more then once in case the surface is being reused if is_managed && is_initial { - add_pre_commit_hook::(&surface, |_, _, surface| { + let _ = add_pre_commit_hook::(&surface, |_, _, surface| { let timestamp = with_states(surface, |states| { states .data_map @@ -226,7 +226,7 @@ where barrier_state.lock().unwrap().register(timestamp) }); - add_blocker(surface, barrier); + let _ = add_blocker(surface, barrier); } }); } diff --git a/src/wayland/compositor/mod.rs b/src/wayland/compositor/mod.rs index c1470411b832..b0760be7b22a 100644 --- a/src/wayland/compositor/mod.rs +++ b/src/wayland/compositor/mod.rs @@ -122,8 +122,8 @@ pub use self::transaction::{Barrier, Blocker, BlockerState}; pub use self::tree::{AlreadyHasRole, TraversalAction}; use self::tree::{PrivateSurfaceData, SuggestedSurfaceState}; pub use crate::utils::hook::HookId; -use crate::utils::Transform; use crate::utils::{user_data::UserDataMap, Buffer, Logical, Point, Rectangle}; +use crate::utils::{DeadResource, Transform}; use atomic_float::AtomicF64; use wayland_server::backend::GlobalId; use wayland_server::protocol::wl_compositor::WlCompositor; @@ -449,7 +449,7 @@ pub fn get_region_attributes(region: &wl_region::WlRegion) -> RegionAttributes { /// post-commit hook to apply state changes (i.e. copy last acked state to current). /// /// Compositors should use this for adding blockers if needed, e.g. the DMA-BUF readiness blocker. -pub fn add_pre_commit_hook(surface: &WlSurface, hook: F) -> HookId +pub fn add_pre_commit_hook(surface: &WlSurface, hook: F) -> Result where F: Fn(&mut D, &DisplayHandle, &WlSurface) + Send + Sync + 'static, D: 'static, @@ -463,11 +463,15 @@ where user_state_type, ); + if !surface.is_alive() { + return Err(DeadResource); + } + let hook = move |state: &mut dyn Any, dh: &DisplayHandle, surface: &WlSurface| { let state = state.downcast_mut::().unwrap(); hook(state, dh, surface); }; - PrivateSurfaceData::add_pre_commit_hook(surface, hook) + Ok(PrivateSurfaceData::add_pre_commit_hook(surface, hook)) } /// Register a post-commit hook to be invoked on surface commit @@ -477,7 +481,7 @@ where /// /// Protocol implementations should apply state changes here, i.e. copy last acked state into /// current. -pub fn add_post_commit_hook(surface: &WlSurface, hook: F) -> HookId +pub fn add_post_commit_hook(surface: &WlSurface, hook: F) -> Result where F: Fn(&mut D, &DisplayHandle, &WlSurface) + Send + Sync + 'static, D: 'static, @@ -491,11 +495,15 @@ where user_state_type, ); + if !surface.is_alive() { + return Err(DeadResource); + } + let hook = move |state: &mut dyn Any, dh: &DisplayHandle, surface: &WlSurface| { let state = state.downcast_mut::().unwrap(); hook(state, dh, surface); }; - PrivateSurfaceData::add_post_commit_hook(surface, hook) + Ok(PrivateSurfaceData::add_post_commit_hook(surface, hook)) } /// Register a destruction hook to be invoked on surface destruction @@ -504,7 +512,7 @@ where /// client disconnect). /// /// D generic is the compositor state, same as used in `CompositorState::new()` -pub fn add_destruction_hook(surface: &WlSurface, hook: F) -> HookId +pub fn add_destruction_hook(surface: &WlSurface, hook: F) -> Result where F: Fn(&mut D, &WlSurface) + Send + Sync + 'static, D: 'static, @@ -518,11 +526,15 @@ where user_state_type, ); + if !surface.is_alive() { + return Err(DeadResource); + } + let hook = move |state: &mut dyn Any, surface: &WlSurface| { let state = state.downcast_mut::().unwrap(); hook(state, surface); }; - PrivateSurfaceData::add_destruction_hook(surface, hook) + Ok(PrivateSurfaceData::add_destruction_hook(surface, hook)) } /// Unregister a pre-commit hook @@ -549,8 +561,12 @@ pub fn remove_destruction_hook(surface: &WlSurface, hook_id: HookId) { /// The module will only evaluate blocker states on commit. If a blocker /// becomes ready later, a call to [`CompositorClientState::blocker_cleared`] is necessary /// to trigger a re-evaluation. -pub fn add_blocker(surface: &WlSurface, blocker: impl Blocker + Send + 'static) { - PrivateSurfaceData::add_blocker(surface, blocker) +pub fn add_blocker(surface: &WlSurface, blocker: impl Blocker + Send + 'static) -> Result<(), DeadResource> { + if !surface.is_alive() { + return Err(DeadResource); + } + PrivateSurfaceData::add_blocker(surface, blocker); + Ok(()) } /// Handler trait for compositor diff --git a/src/wayland/drm_syncobj/mod.rs b/src/wayland/drm_syncobj/mod.rs index 6b19cac3d22a..768fb14d0bae 100644 --- a/src/wayland/drm_syncobj/mod.rs +++ b/src/wayland/drm_syncobj/mod.rs @@ -308,9 +308,15 @@ where ); return; } - let commit_hook_id = compositor::add_pre_commit_hook::(&surface, commit_hook); - let destruction_hook_id = - compositor::add_destruction_hook::(&surface, destruction_hook); + let Ok(commit_hook_id) = compositor::add_pre_commit_hook::(&surface, commit_hook) + else { + return; + }; + let Ok(destruction_hook_id) = + compositor::add_destruction_hook::(&surface, destruction_hook) + else { + return; + }; let syncobj_surface = data_init.init::<_, _>( id, DrmSyncobjSurfaceData { diff --git a/src/wayland/fifo/mod.rs b/src/wayland/fifo/mod.rs index 64e534b99f89..87bad28c26b6 100644 --- a/src/wayland/fifo/mod.rs +++ b/src/wayland/fifo/mod.rs @@ -206,7 +206,7 @@ where // Make sure we do not install the hook more then once in case the surface is being reused if is_managed && is_initial { - add_pre_commit_hook::(&surface, |_, _, surface| { + let _ = add_pre_commit_hook::(&surface, |_, _, surface| { let fifo_barrier = with_states(surface, |states| { let fifo_state = *states.cached_state.get::().pending(); @@ -253,7 +253,7 @@ where // sync subsurfaces let skip = barrier.is_signaled() || is_sync_subsurface(surface); if !skip { - add_blocker(surface, barrier); + let _ = add_blocker(surface, barrier); } } }); diff --git a/src/wayland/pointer_constraints.rs b/src/wayland/pointer_constraints.rs index cba36926752f..e2d289efca65 100644 --- a/src/wayland/pointer_constraints.rs +++ b/src/wayland/pointer_constraints.rs @@ -340,7 +340,7 @@ fn add_constraint( }); if added { - compositor::add_post_commit_hook(surface, commit_hook::); + let _ = compositor::add_post_commit_hook(surface, commit_hook::); } } diff --git a/src/wayland/seat/keyboard.rs b/src/wayland/seat/keyboard.rs index 2b78404a8a93..1c6525d0683a 100644 --- a/src/wayland/seat/keyboard.rs +++ b/src/wayland/seat/keyboard.rs @@ -210,16 +210,14 @@ pub(crate) fn enter_internal( keys: impl Iterator, serial: Serial, ) { - *seat.get_keyboard().unwrap().arc.last_enter.lock().unwrap() = Some(serial); - let serialized_keys = serialize_pressed_keys(keys); - for_each_focused_kbds(seat, surface, |kbd| { - kbd.enter(serial.into(), surface, serialized_keys.clone()) - }); + let seat_clone = seat.downgrade(); + let Ok(hook_id) = add_destruction_hook::(surface, move |_, surface| { + let Some(seat) = seat_clone.upgrade() else { + return; + }; - let seat_clone = seat.clone(); - let hook_id = add_destruction_hook::(surface, move |_, surface| { if let Some(client) = surface.client() { - let keyboard = seat_clone.get_keyboard().unwrap(); + let keyboard = seat.get_keyboard().unwrap(); let inner = keyboard.arc.known_kbds.lock().unwrap(); for kbd in &*inner { let Ok(kbd) = kbd.upgrade() else { @@ -231,7 +229,16 @@ pub(crate) fn enter_internal( } } } + }) else { + return; + }; + + *seat.get_keyboard().unwrap().arc.last_enter.lock().unwrap() = Some(serial); + let serialized_keys = serialize_pressed_keys(keys); + for_each_focused_kbds(seat, surface, |kbd| { + kbd.enter(serial.into(), surface, serialized_keys.clone()) }); + if let Some(old_hook_id) = with_states(surface, |states| { states .data_map diff --git a/src/wayland/session_lock/lock.rs b/src/wayland/session_lock/lock.rs index 9f4492a7e00b..3efd86cbea9e 100644 --- a/src/wayland/session_lock/lock.rs +++ b/src/wayland/session_lock/lock.rs @@ -81,7 +81,7 @@ where let lock_surface = data_init.init(id, data); // Initialize surface data. - compositor::with_states(&surface, |states| { + let initial = compositor::with_states(&surface, |states| { let inserted = states.data_map.insert_if_missing_threadsafe(|| { Mutex::new(LockSurfaceAttributes::new(lock_surface.clone())) }); @@ -95,10 +95,14 @@ where .unwrap(); attributes.surface = lock_surface.clone(); } + + inserted }); - // Add pre-commit hook for updating surface state. - compositor::add_pre_commit_hook::(&surface, LockSurface::pre_commit_hook); + if initial { + // Add pre-commit hook for updating surface state. + let _ = compositor::add_pre_commit_hook::(&surface, LockSurface::pre_commit_hook); + } // Call compositor handler. let lock_surface = LockSurface::new(surface, lock_surface); diff --git a/src/wayland/shell/wlr_layer/handlers.rs b/src/wayland/shell/wlr_layer/handlers.rs index 73e73a7ae87d..b415761eef31 100644 --- a/src/wayland/shell/wlr_layer/handlers.rs +++ b/src/wayland/shell/wlr_layer/handlers.rs @@ -122,7 +122,7 @@ where }); if initial { - compositor::add_pre_commit_hook::( + let _ = compositor::add_pre_commit_hook::( &wl_surface, super::LayerSurface::pre_commit_hook, ); diff --git a/src/wayland/shell/xdg/handlers/surface.rs b/src/wayland/shell/xdg/handlers/surface.rs index f89cd46e338e..83365a18f364 100644 --- a/src/wayland/shell/xdg/handlers/surface.rs +++ b/src/wayland/shell/xdg/handlers/surface.rs @@ -124,7 +124,7 @@ where }); if initial { - compositor::add_pre_commit_hook::( + let _ = compositor::add_pre_commit_hook::( surface, super::super::ToplevelSurface::pre_commit_hook, ); @@ -202,7 +202,7 @@ where }); if initial { - compositor::add_pre_commit_hook::( + let _ = compositor::add_pre_commit_hook::( surface, super::super::PopupSurface::pre_commit_hook, ); diff --git a/src/wayland/viewporter/mod.rs b/src/wayland/viewporter/mod.rs index a176c5515f10..8986a7cdee99 100644 --- a/src/wayland/viewporter/mod.rs +++ b/src/wayland/viewporter/mod.rs @@ -177,7 +177,7 @@ where // only add the pre-commit hook once for the surface if initial { - compositor::add_pre_commit_hook::(&surface, viewport_pre_commit_hook); + let _ = compositor::add_pre_commit_hook::(&surface, viewport_pre_commit_hook); } } wp_viewporter::Request::Destroy => { diff --git a/src/wayland/xwayland_shell.rs b/src/wayland/xwayland_shell.rs index 2961a263542f..1572b1e8db5c 100644 --- a/src/wayland/xwayland_shell.rs +++ b/src/wayland/xwayland_shell.rs @@ -264,7 +264,9 @@ where return; } - compositor::add_pre_commit_hook::(&surface, serial_commit_hook); + // TODO: Can the xwayland shell role be re-used? In this case we should guard the pre-commit hook + // to avoid duplicates. + let _ = compositor::add_pre_commit_hook::(&surface, serial_commit_hook); data_init.init(id, XWaylandSurfaceUserData { wl_surface: surface }); // We call the handler callback once the serial is set.