From f6f5b98235b1a5052aff1fb5a48e7951720beb42 Mon Sep 17 00:00:00 2001 From: Kirill Chibisov Date: Sat, 12 Dec 2020 18:28:29 +0300 Subject: [PATCH] Fix segfault in multithreaded context Dropping libwayland-client's queue in multithreaded context is safe if and only if there're no alive proxies on it, otherwise it could result in use-after-free. For more see [1] and [2]. [1] - https://gitlab.freedesktop.org/wayland/wayland/-/issues/13 [2] - https://github.com/Smithay/wayland-rs/issues/359 --- CHANGELOG.md | 2 ++ examples/clipboard.rs | 2 +- src/worker/dispatch_data.rs | 71 ++++++++++++++++++++++++------------- src/worker/handlers.rs | 10 +++--- src/worker/mod.rs | 45 +++++++++++++++++------ 5 files changed, 88 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35bf823..90d7261 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Segfault when dropping clipboard in multithreaded context while main queue is still running + ## 0.6.1 -- 2020-10-13 - Crash when failing to write to a clipboard diff --git a/examples/clipboard.rs b/examples/clipboard.rs index 3b17a6b..bd15f0e 100644 --- a/examples/clipboard.rs +++ b/examples/clipboard.rs @@ -223,7 +223,7 @@ fn process_keyboard_event(event: KeyboardEvent, dispatch_data: &mut DispatchData let contents = dispatch_data .clipboard .load() - .unwrap_or_else(|_| String::from("Failed to load primary selection")); + .unwrap_or_else(|_| String::from("Failed to load selection")); println!("Paste: {}", contents); } // Copy primary. diff --git a/src/worker/dispatch_data.rs b/src/worker/dispatch_data.rs index 7411ef3..be6fef7 100644 --- a/src/worker/dispatch_data.rs +++ b/src/worker/dispatch_data.rs @@ -1,47 +1,68 @@ +use std::collections::VecDeque; +use std::slice::IterMut; + use sctk::reexports::client::protocol::wl_seat::WlSeat; +use super::seat_data::SeatData; + /// Data to track latest seat and serial for clipboard requests. -#[derive(Default)] pub struct ClipboardDispatchData { - observed_seats: Vec<(WlSeat, u32)>, - last_pos: usize, + /// Seats that our application encountered. The first seat is the latest one we've encountered. + observed_seats: VecDeque<(WlSeat, u32)>, + + /// All the seats that were advertised. + seats: Vec, } impl ClipboardDispatchData { /// Builds new `ClipboardDispatchData` with all fields equal to `None`. - pub fn new() -> Self { - Self::default() + pub fn new(seats: Vec) -> Self { + Self { observed_seats: Default::default(), seats } + } + + /// Returns the requested seat's data or adds a new one. + pub fn get_seat_data_or_add(&mut self, seat: WlSeat) -> &mut SeatData { + let pos = self.seats.iter().position(|st| st.seat == seat); + let index = pos.unwrap_or_else(|| { + self.seats.push(SeatData::new(seat, None, None)); + self.seats.len() - 1 + }); + + &mut self.seats[index] + } + + pub fn seats(&mut self) -> IterMut<'_, SeatData> { + self.seats.iter_mut() } /// Set the last observed seat. - pub fn set_last_seat(&mut self, seat: WlSeat, serial: u32) { + pub fn set_last_observed_seat(&mut self, seat: WlSeat, serial: u32) { let pos = self.observed_seats.iter().position(|st| st.0 == seat); - match pos { + let (seat, serial) = match pos { Some(pos) => { - // Update serial and set the last data we've seen. - self.observed_seats[pos].1 = serial; - self.last_pos = pos; + // We just found that `pos` we're going to remove, so unwrapping is safe. + self.observed_seats.remove(pos).unwrap() } - None => { - // Add new seat and mark it as last. - self.last_pos = self.observed_seats.len(); - self.observed_seats.push((seat, serial)); - } - } + None => (seat, serial), + }; + + // Add seat to front, thus it'll be the latest observed one. + self.observed_seats.push_front((seat, serial)); } - /// Remove the given seat from the observer seats. - pub fn remove_seat(&mut self, seat: WlSeat) { - let pos = self.observed_seats.iter().position(|st| st.0 == seat); + /// Remove the given seat from the observed seats. + pub fn remove_observed_seat(&mut self, seat: WlSeat) { + let pos = match self.observed_seats.iter().position(|st| st.0 == seat) { + Some(pos) => pos, + None => return, + }; - if let Some(pos) = pos { - // Remove the seat data. - self.observed_seats.remove(pos); - } + // Remove the seat data. + self.observed_seats.remove(pos); } /// Return the last observed seat and the serial. - pub fn last_seat(&self) -> Option<&(WlSeat, u32)> { - self.observed_seats.get(self.last_pos) + pub fn last_observed_seat(&self) -> Option<&(WlSeat, u32)> { + self.observed_seats.front() } } diff --git a/src/worker/handlers.rs b/src/worker/handlers.rs index 8e197dd..3c0afd8 100644 --- a/src/worker/handlers.rs +++ b/src/worker/handlers.rs @@ -104,10 +104,10 @@ pub fn pointer_handler(seat: WlSeat, event: PointerEvent, mut dispatch_data: Dis }; match event { PointerEvent::Enter { serial, .. } => { - dispatch_data.set_last_seat(seat, serial); + dispatch_data.set_last_observed_seat(seat, serial); } PointerEvent::Button { serial, .. } => { - dispatch_data.set_last_seat(seat, serial); + dispatch_data.set_last_observed_seat(seat, serial); } _ => {} } @@ -121,13 +121,13 @@ pub fn keyboard_handler(seat: WlSeat, event: KeyboardEvent, mut dispatch_data: D }; match event { KeyboardEvent::Enter { serial, .. } => { - dispatch_data.set_last_seat(seat, serial); + dispatch_data.set_last_observed_seat(seat, serial); } KeyboardEvent::Key { serial, .. } => { - dispatch_data.set_last_seat(seat, serial); + dispatch_data.set_last_observed_seat(seat, serial); } KeyboardEvent::Leave { .. } => { - dispatch_data.remove_seat(seat); + dispatch_data.remove_observed_seat(seat); } KeyboardEvent::Keymap { fd, .. } => { // Prevent fd leaking. diff --git a/src/worker/mod.rs b/src/worker/mod.rs index 3fc11f6..c7968fc 100644 --- a/src/worker/mod.rs +++ b/src/worker/mod.rs @@ -143,15 +143,13 @@ fn worker_impl(display: Display, request_rx: Receiver, reply_tx: Sender } // Listen for seats. - let _listener = env.listen_for_seats(move |seat, seat_data, _| { - let detached_seat = seat.detach(); - let pos = seats.iter().position(|st| st.seat == detached_seat); - let index = pos.unwrap_or_else(|| { - seats.push(SeatData::new(detached_seat, None, None)); - seats.len() - 1 - }); + let listener = env.listen_for_seats(move |seat, seat_data, mut dispatch_data| { + let dispatch_data = match dispatch_data.get::() { + Some(dispatch_data) => dispatch_data, + None => return, + }; - let seat_resources = &mut seats[index]; + let seat_resources = dispatch_data.get_seat_data_or_add(seat.detach()); if seat_data.has_keyboard && !seat_data.defunct { if seat_resources.keyboard.is_none() { @@ -194,7 +192,7 @@ fn worker_impl(display: Display, request_rx: Receiver, reply_tx: Sender // Flush the display. let _ = queue.display().flush(); - let mut dispatch_data = ClipboardDispatchData::new(); + let mut dispatch_data = ClipboardDispatchData::new(seats); // Setup sleep amount tracker. let mut sa_tracker = SleepAmountTracker::new(MAX_TIME_TO_SLEEP, MAX_WARM_WAKEUPS); @@ -219,7 +217,7 @@ fn worker_impl(display: Display, request_rx: Receiver, reply_tx: Sender } // Get latest observed seat and serial. - let (seat, serial) = match dispatch_data.last_seat() { + let (seat, serial) = match dispatch_data.last_observed_seat() { Some(data) => data, None => { handlers::reply_error(&reply_tx, "no focus on a seat."); @@ -284,7 +282,7 @@ fn worker_impl(display: Display, request_rx: Receiver, reply_tx: Sender // If some application is trying to spam us when there're no seats, it's likely that // someone is trying to paste from us. - if dispatch_data.last_seat().is_none() && pending_events != 0 { + if dispatch_data.last_observed_seat().is_none() && pending_events != 0 { sa_tracker.reset_sleep(); } else { // Time for thread to sleep. @@ -296,4 +294,29 @@ fn worker_impl(display: Display, request_rx: Receiver, reply_tx: Sender sa_tracker.increase_sleep(); } } + + // While everything inside this block is safe, the logic is generally unsafe, since we must + // drop every proxy on the current `queue`, since dropping it in multithreaded context + // could result in use-after-free in libwayland-client. + // + // For more see https://gitlab.freedesktop.org/wayland/wayland/-/issues/13. + #[allow(unused_unsafe)] + unsafe { + for seat in dispatch_data.seats() { + if let Some(pointer) = seat.pointer.take() { + if pointer.as_ref().version() >= 3 { + pointer.release(); + } + } + if let Some(keyboard) = seat.keyboard.take() { + if keyboard.as_ref().version() >= 3 { + keyboard.release(); + } + } + } + std::mem::drop(listener); + + let _ = queue.sync_roundtrip(&mut dispatch_data, |_, _, _| unimplemented!()); + let _ = queue.display().flush(); + } }