diff --git a/crates/pf-capture/src/linux/mod.rs b/crates/pf-capture/src/linux/mod.rs index ca340e5c..8e57d8a9 100644 --- a/crates/pf-capture/src/linux/mod.rs +++ b/crates/pf-capture/src/linux/mod.rs @@ -1310,21 +1310,26 @@ mod pipewire { /// (which Mutter delivers as metadata-only "corrupted" buffers) still refresh the position. fn update_cursor_meta(cursor: &mut CursorState, spa_buf: *mut spa::sys::spa_buffer) { // SAFETY: `spa_buf` is the live buffer we still hold (dequeued, not yet requeued). - // `spa_buffer_find_meta_data` scans its metadata array for a `SPA_META_Cursor` of at least - // `size_of::()` bytes and returns a pointer into that buffer's metadata - // (or null), valid until requeue. The size argument matches the struct the result is cast to. - let cur = unsafe { - spa::sys::spa_buffer_find_meta_data( - spa_buf, - spa::sys::SPA_META_Cursor, - std::mem::size_of::(), - ) as *const spa::sys::spa_meta_cursor - }; - if cur.is_null() { + // `spa_buffer_find_meta` returns the `spa_meta` (type + byte `size` + `data` pointer) for + // `SPA_META_Cursor`, or null. We take `find_meta` rather than `find_meta_data` specifically + // to obtain the region's real `size`: the bitmap offset, pixel offset and stride read below + // are ALL producer-written, and without a bound against the actual region they drive + // out-of-bounds pointer arithmetic and an oversized `slice::from_raw_parts` — an OOB read + // that SIGSEGVs inside the PipeWire `.process` callback (a segfault `catch_unwind` cannot + // catch). Every offset below is validated against `region_size` with checked arithmetic, + // mirroring the fd-length guard the main frame path already applies to xdg-desktop-portal-wlr. + let meta = + unsafe { spa::sys::spa_buffer_find_meta(spa_buf, spa::sys::SPA_META_Cursor as u32) }; + if meta.is_null() { return; } - // SAFETY: `cur` is non-null and points to a `spa_meta_cursor` of at least its own size - // inside the held buffer (guaranteed by the size arg above), so every field read is in bounds. + // SAFETY: `meta` is non-null and points into the held buffer's metadata array. + let (region_size, data) = unsafe { ((*meta).size as usize, (*meta).data as *const u8) }; + if data.is_null() || region_size < std::mem::size_of::() { + return; + } + let cur = data as *const spa::sys::spa_meta_cursor; + // SAFETY: `region_size >= size_of::()` checked above, so every field is in bounds. let (id, pos_x, pos_y, hot_x, hot_y, bmp_off) = unsafe { ( (*cur).id, @@ -1347,13 +1352,18 @@ mod pipewire { // Position-only update — keep the cached bitmap. return; } - // SAFETY: `bitmap_offset` is a byte offset from `cur` to a `spa_meta_bitmap`, which the - // producer placed inside the same meta region it sized for this cursor (>= the size we - // requested). The resulting pointer is in bounds and aligned for `spa_meta_bitmap`. - let bmp = - unsafe { (cur as *const u8).add(bmp_off as usize) as *const spa::sys::spa_meta_bitmap }; - // SAFETY: `bmp` is the in-bounds, aligned `spa_meta_bitmap` pointer computed just above; the - // producer fully initialized this header, so reading its scalar fields is sound. + let bmp_off = bmp_off as usize; + // The `spa_meta_bitmap` header must fit entirely inside the region before we read it — + // `bitmap_offset` is producer-controlled and otherwise reads past the metadata. + match bmp_off.checked_add(std::mem::size_of::()) { + Some(end) if end <= region_size => {} + _ => return, + } + // SAFETY: `bmp_off + size_of::() <= region_size` (checked directly above), + // so the header is fully in bounds; the producer places it aligned as before. + let bmp = unsafe { data.add(bmp_off) as *const spa::sys::spa_meta_bitmap }; + // SAFETY: `bmp` is the in-bounds `spa_meta_bitmap` header validated just above; reading its + // scalar fields is sound. let (vfmt, bw, bh, stride, pix_off) = unsafe { ( (*bmp).format, @@ -1369,10 +1379,27 @@ mod pipewire { } let row = bw as usize * 4; let stride = if stride < row { row } else { stride }; - let span = stride * (bh as usize - 1) + row; - // SAFETY: the bitmap pixels live at `bmp + pix_off` for `span` bytes, within the - // producer-sized meta region. `span` is the exact extent the strided copy below reads. - let src = unsafe { std::slice::from_raw_parts((bmp as *const u8).add(pix_off), span) }; + // `span` is the exact byte extent the strided loop reads: `stride·(bh-1) + row`. Compute it + // with checked arithmetic (a producer stride near `i32::MAX` would otherwise overflow) and + // require the whole pixel block `[bmp_off + pix_off, +span)` to lie inside the region before + // fabricating the slice — this is the check whose absence made the read go out of bounds. + let span = match stride + .checked_mul(bh as usize - 1) + .and_then(|v| v.checked_add(row)) + { + Some(s) => s, + None => return, + }; + match bmp_off + .checked_add(pix_off) + .and_then(|v| v.checked_add(span)) + { + Some(end) if end <= region_size => {} + _ => return, + } + // SAFETY: `bmp_off + pix_off + span <= region_size` (checked directly above), so the slice + // is fully within the producer's meta region; `span` is exactly the strided loop's extent. + let src = unsafe { std::slice::from_raw_parts(data.add(bmp_off + pix_off), span) }; let mut rgba = vec![0u8; bw as usize * bh as usize * 4]; for y in 0..bh as usize { for x in 0..bw as usize { diff --git a/crates/pf-inject/src/inject/linux/steam_gadget.rs b/crates/pf-inject/src/inject/linux/steam_gadget.rs index e328640d..c734b1bb 100644 --- a/crates/pf-inject/src/inject/linux/steam_gadget.rs +++ b/crates/pf-inject/src/inject/linux/steam_gadget.rs @@ -17,7 +17,7 @@ use anyhow::{bail, Context, Result}; use std::mem::size_of; use std::os::fd::RawFd; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; @@ -196,6 +196,41 @@ impl Drop for GadgetFd { } } +/// The signal used to break a worker thread out of a blocking raw_gadget ioctl at teardown. +/// `EVENT_FETCH`/`EP_WRITE` are `wait_event_interruptible` in the kernel with no timeout and no +/// `O_NONBLOCK` honouring, and closing the fd cannot wake a thread already inside the ioctl (the +/// in-flight syscall holds a reference to the struct file). A signal is the only reliable lever: +/// delivered with a no-op, non-`SA_RESTART` handler it forces the ioctl to return `EINTR`, after +/// which the loop's top-of-iteration `running` check exits. `SIGUSR1` is unused elsewhere in this +/// process; the handler is a no-op, so a stray `SIGUSR1` becomes harmless rather than fatal. +const WAKE_SIGNAL: libc::c_int = libc::SIGUSR1; + +/// Install the no-op `WAKE_SIGNAL` handler exactly once. Crucially `sa_flags = 0` (no `SA_RESTART`) +/// so a delivered signal makes the interruptible ioctl return `EINTR` instead of auto-restarting. +fn install_wake_handler() { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + extern "C" fn noop(_: libc::c_int) {} + // SAFETY: installing a well-formed `sigaction` with an empty mask and a valid no-op handler + // for a single signal; touches only this process's disposition for `WAKE_SIGNAL`. + unsafe { + let mut sa: libc::sigaction = std::mem::zeroed(); + sa.sa_sigaction = noop as usize; + libc::sigemptyset(&mut sa.sa_mask); + sa.sa_flags = 0; + libc::sigaction(WAKE_SIGNAL, &sa, std::ptr::null_mut()); + } + }); +} + +/// Lets `Drop` wake a specific worker thread parked in a blocking ioctl. `tid` is the thread's +/// `pthread_self()` (0 until it starts); `done` is set right before the thread returns, so `Drop` +/// stops signalling a thread that has already exited. +struct Waker { + tid: Arc, + done: Arc, +} + /// A virtual Steam Deck presented over the USB gadget subsystem. Dropping it stops the threads and /// closes the gadget (the kernel tears down the device). pub struct SteamDeckGadget { @@ -203,6 +238,7 @@ pub struct SteamDeckGadget { feedback: Arc>, running: Arc, threads: Vec>, + wakers: Vec, _fd: Arc, seq: u32, } @@ -243,6 +279,18 @@ impl SteamDeckGadget { let ctrl_ep = Arc::new(std::sync::atomic::AtomicI32::new(-1)); let configured = Arc::new(AtomicBool::new(false)); + // The teardown wake path (see `WAKE_SIGNAL`) needs the handler installed before any thread + // can park in a blocking ioctl. + install_wake_handler(); + let ctrl_waker = Waker { + tid: Arc::new(AtomicU64::new(0)), + done: Arc::new(AtomicBool::new(false)), + }; + let stream_waker = Waker { + tid: Arc::new(AtomicU64::new(0)), + done: Arc::new(AtomicBool::new(false)), + }; + // Control thread: enumerate + answer every control transfer. let control = { let fd = fd.clone(); @@ -250,10 +298,15 @@ impl SteamDeckGadget { let ctrl_ep = ctrl_ep.clone(); let configured = configured.clone(); let feedback = feedback.clone(); + let tid = ctrl_waker.tid.clone(); + let done = ctrl_waker.done.clone(); std::thread::Builder::new() .name("pf-deck-gadget-ctrl".into()) .spawn(move || { - control_loop(fd, running, ctrl_ep, configured, feedback, serial, unit_id) + // SAFETY: `pthread_self` is always valid on the calling thread. + tid.store(unsafe { libc::pthread_self() } as u64, Ordering::SeqCst); + control_loop(fd, running, ctrl_ep, configured, feedback, serial, unit_id); + done.store(true, Ordering::SeqCst); }) .context("spawn gadget control thread")? }; @@ -264,9 +317,16 @@ impl SteamDeckGadget { let ctrl_ep = ctrl_ep.clone(); let configured = configured.clone(); let report = report.clone(); + let tid = stream_waker.tid.clone(); + let done = stream_waker.done.clone(); std::thread::Builder::new() .name("pf-deck-gadget-stream".into()) - .spawn(move || stream_loop(fd, running, ctrl_ep, configured, report)) + .spawn(move || { + // SAFETY: `pthread_self` is always valid on the calling thread. + tid.store(unsafe { libc::pthread_self() } as u64, Ordering::SeqCst); + stream_loop(fd, running, ctrl_ep, configured, report); + done.store(true, Ordering::SeqCst); + }) .context("spawn gadget stream thread")? }; @@ -275,6 +335,7 @@ impl SteamDeckGadget { feedback, running, threads: vec![control, stream], + wakers: vec![ctrl_waker, stream_waker], _fd: fd, seq: 0, }) @@ -302,6 +363,32 @@ impl SteamDeckGadget { impl Drop for SteamDeckGadget { fn drop(&mut self) { self.running.store(false, Ordering::SeqCst); + // The control thread spends steady state parked in a blocking `EVENT_FETCH` ioctl that only + // tests `running` at the top of its loop, so clearing the flag is not enough — it must be + // signalled out of the syscall (see `WAKE_SIGNAL`). Without this the join below can hang the + // caller (the session input thread, via `PadSlots::sweep`) indefinitely. Retry until each + // thread reports done, to cover the race where the signal lands just before the thread + // re-enters the ioctl; bounded (~1 s) so a genuinely stuck thread can't wedge teardown either. + for _ in 0..200 { + let mut all_done = true; + for w in &self.wakers { + if w.done.load(Ordering::SeqCst) { + continue; + } + all_done = false; + let tid = w.tid.load(Ordering::SeqCst); + if tid != 0 { + // SAFETY: the thread is joinable and not yet joined (join runs after this loop), + // so `tid` names a live pthread; `pthread_kill` on a finished-but-unjoined thread + // is defined (returns ESRCH), never UB. + unsafe { libc::pthread_kill(tid as libc::pthread_t, WAKE_SIGNAL) }; + } + } + if all_done { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } for t in self.threads.drain(..) { let _ = t.join(); } diff --git a/crates/pf-zerocopy/src/imp/cuda.rs b/crates/pf-zerocopy/src/imp/cuda.rs index 4e9a0e3f..4b80cc04 100644 --- a/crates/pf-zerocopy/src/imp/cuda.rs +++ b/crates/pf-zerocopy/src/imp/cuda.rs @@ -1003,7 +1003,13 @@ impl RegisteredTexture { // SAFETY: `self.resource` is the valid `CUgraphicsResource` from a successful `register_gl` // (its only constructor), so the wrappers forward to the live table; the caller holds the // GL+CUDA contexts current (the registration's contract). `cuGraphicsMapResources` maps - // `count == 1` resource via `&mut self.resource` (a live field) on the default stream; + // `count == 1` resource via `&mut self.resource` (a live field). It is issued on + // `copy_stream()` — NOT the NULL stream — because map's only ordering guarantee is that + // prior GL work completes before subsequent CUDA work issued IN THE STREAM PASSED TO IT; + // the copy below runs on `copy_stream()` (a `CU_STREAM_NON_BLOCKING` stream, exempt from + // implicit NULL-stream ordering), so mapping on NULL left the copy free to race the GL + // de-tile/CSC that produced this texture (glFlush only, no fence) — intermittent torn or + // stale frames under GPU load. Map, copy, and unmap now all share `copy_stream()`. // `cuGraphicsSubResourceGetMappedArray` writes the mapped `CUarray` into the live local // `array` (index 0, mip 0). On failure we unmap and bail (balanced). `©` is a live // local `CUDA_MEMCPY2D` outliving the synchronous `copy_blocking`: `srcArray` is valid @@ -1012,12 +1018,12 @@ impl RegisteredTexture { // we always unmap afterward (even on error), keeping the map/unmap pair balanced. unsafe { ck( - cuGraphicsMapResources(1, &mut self.resource, std::ptr::null_mut()), + cuGraphicsMapResources(1, &mut self.resource, copy_stream()), "cuGraphicsMapResources", )?; let mut array: CUarray = std::ptr::null_mut(); if cuGraphicsSubResourceGetMappedArray(&mut array, self.resource, 0, 0) != 0 { - let _ = cuGraphicsUnmapResources(1, &mut self.resource, std::ptr::null_mut()); + let _ = cuGraphicsUnmapResources(1, &mut self.resource, copy_stream()); bail!("cuGraphicsSubResourceGetMappedArray failed"); } let copy = CUDA_MEMCPY2D { @@ -1031,7 +1037,7 @@ impl RegisteredTexture { ..Default::default() }; let res = copy_blocking(©, "cuMemcpy2DAsync_v2"); - let _ = cuGraphicsUnmapResources(1, &mut self.resource, std::ptr::null_mut()); + let _ = cuGraphicsUnmapResources(1, &mut self.resource, copy_stream()); res } } @@ -1058,12 +1064,12 @@ impl RegisteredTexture { // so the map/unmap pair stays balanced and the array outlives the copy. unsafe { ck( - cuGraphicsMapResources(1, &mut self.resource, std::ptr::null_mut()), + cuGraphicsMapResources(1, &mut self.resource, copy_stream()), "cuGraphicsMapResources", )?; let mut array: CUarray = std::ptr::null_mut(); if cuGraphicsSubResourceGetMappedArray(&mut array, self.resource, 0, 0) != 0 { - let _ = cuGraphicsUnmapResources(1, &mut self.resource, std::ptr::null_mut()); + let _ = cuGraphicsUnmapResources(1, &mut self.resource, copy_stream()); bail!("cuGraphicsSubResourceGetMappedArray failed"); } let copy = CUDA_MEMCPY2D { @@ -1077,7 +1083,7 @@ impl RegisteredTexture { ..Default::default() }; let res = copy_blocking(©, "cuMemcpy2DAsync_v2(plane)"); - let _ = cuGraphicsUnmapResources(1, &mut self.resource, std::ptr::null_mut()); + let _ = cuGraphicsUnmapResources(1, &mut self.resource, copy_stream()); res } }