From a62c93fc0d10b86ab650799f23941d697531e555 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Fri, 31 Jul 2026 12:30:31 -0300 Subject: [PATCH] drm: fix the ABI cross-check's path, and stop panicking on a failed spawn The ABI cross-check added in the previous commit could never run: both callers of stage_libdrmtap_into_deb chdir into flutter/ first, and the check opened drmtap_dl.rs by a path relative to the cwd, so every --drm packaging run died with FileNotFoundError. CI caught it. It is anchored on __file__ now, and read through a context manager. Worth naming why the test missed it: the check was exercised from the repository root, which is the one directory where the bug is invisible. A control that does not reproduce the call site's conditions is not a control. Three more, all the same class the previous commit was already fixing - a hazard closed at one site and left at its siblings: - `std::thread::spawn` panics when the thread cannot be created, and the panic unwinds into whoever called it. The two hardened workers used Builder; the five remaining DRM threads did not. The startup ones now log and degrade (a lost pre-warm costs one cold probe, a lost udev listener costs the mid-session push, a lost warm costs the first session), and the two per-session ones live in functions that already return ResultType, so they fail that one connection cleanly instead of unwinding through the handler. - The wire descriptor's `num_planes` was clamped to 1..=4 here while `drm_render::convert` rejects an out-of-range count on purpose, so that the count the C reads is the count this side validated. Clamping made that reject unreachable: a descriptor claiming 7 planes arrived as 4 and passed. The two guards were added by different review rounds and had been quietly cancelling each other. The raw value is passed through now, leaving one validation site, next to the code that dereferences it. - A SAFETY comment claimed the cursor is released only on success. It is released on every path after a successful get_cursor; only a failed get_cursor returns without releasing, because then there is nothing to release. The release protocol is the reason that block is unsafe, so the comment describing it has to be right. --- build.py | 8 +++++++- libs/scrap/src/common/drm_reader.rs | 5 ++++- src/ipc/drm.rs | 29 ++++++++++++++++++++++++++--- src/server.rs | 10 +++++++++- src/server/drm_capturer.rs | 19 ++++++++++++++----- 5 files changed, 60 insertions(+), 11 deletions(-) diff --git a/build.py b/build.py index 43f203258..c7a43b447 100755 --- a/build.py +++ b/build.py @@ -530,7 +530,13 @@ def assert_so_satisfies_the_runtime_abi_gate(so_path): print(f'[drm] cannot read a version out of {so_path}; skipping the ABI-gate cross-check') return so_ver = tuple(int(g) for g in m.groups()) - gate_src = open('libs/scrap/src/common/drmtap_dl.rs').read() + # Anchored on THIS file, not on the cwd: both callers of stage_libdrmtap_into_deb have already + # chdir'd into flutter/ by the time they get here, so a cwd-relative path raises FileNotFoundError + # and fails every --drm packaging run. (It did; CI caught it.) + gate_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), + 'libs', 'scrap', 'src', 'common', 'drmtap_dl.rs') + with open(gate_path) as f: + gate_src = f.read() def _const(name): mm = re.search(rf'const {name}: c_int = (\d+);', gate_src) diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 3a54ecc44..a98470980 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -477,7 +477,10 @@ impl DrmReader { /// reports the cursor invisible, the real shape when visible, or None on a /// read error / unsupported cursor. Ported from the old drm.rs update_cursor. pub fn cursor(&mut self) -> Option { - // SAFETY: ctx valid; c zeroed before the call; released only on success. + // SAFETY: ctx valid; c zeroed before the call; released on EVERY path after a + // successful get_cursor -- the hidden-cursor sentinel and the rejected-geometry arm + // included. Only a failed get_cursor (cret != 0) returns without releasing, because then + // there is nothing to release. The release protocol is why this block is unsafe. unsafe { let mut c: drmtap_cursor_info = std::mem::zeroed(); let cret = (self.lib.get_cursor)(self.ctx, &mut c); diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index ae0f638f6..e4b84e87c 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -1064,11 +1064,28 @@ pub async fn start_drm() { // outlives sessions, a later Wayland login must find the `_drm` socket, and every // handshake enumerates fresh (drm_enumerate_settled), so a skipped prewarm costs that // session only the one-time library/EGL warmup the prewarm exists to hide. - std::thread::spawn(drm_prewarm); + // Builder, not `thread::spawn`: the latter PANICS if the thread cannot be created + // (EAGAIN under a thread-count or memory limit), and that panic would unwind out of + // `start_drm` and take the `_drm` listener with it -- losing the whole feature entry + // point to a failure whose only cost should be one best-effort task. + if let Err(err) = std::thread::Builder::new() + .name("drm-prewarm".into()) + .spawn(drm_prewarm) + { + log::warn!("drm: could not spawn the pre-warm thread ({err}); skipping the warmup"); + } // Watch for connector hotplug/modeset uevents so a mid-session topology change refreshes // the display cache and is pushed to live consumers (best-effort; own thread since it // blocks on recv and re-enumeration is a blocking `!Send` open). - std::thread::spawn(drm_udev_listener); + if let Err(err) = std::thread::Builder::new() + .name("drm-udev".into()) + .spawn(drm_udev_listener) + { + log::warn!( + "drm: could not spawn the udev listener ({err}); a mid-session topology change \ + will not be pushed, and consumers pick it up on their next handshake" + ); + } loop { match incoming.next().await { Some(Ok(stream)) => { @@ -1246,7 +1263,13 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { // on the dma-buf path) inside the privileged service for a consumer that is behind. let frames_gated = Arc::new(AtomicBool::new(false)); let worker_gate = frames_gated.clone(); - std::thread::spawn(move || drm_capture_worker(frame_tx, crtc_rx, worker_stop, worker_gate)); + // Builder for the same reason as the startup threads, and here the caller is an async task + // that already returns ResultType: a spawn failure ends this one connection with an error + // instead of unwinding a panic through the connection handler. + std::thread::Builder::new() + .name("drm-capture".into()) + .spawn(move || drm_capture_worker(frame_tx, crtc_rx, worker_stop, worker_gate)) + .map_err(|err| anyhow::anyhow!("could not spawn the drm capture worker: {err}"))?; // Handshake: the worker sends the display list -- a fresh, settled enumeration // (drm_enumerate_settled), possibly held back while a display wake completes. A closed channel diff --git a/src/server.rs b/src/server.rs index 4029ed453..5af982772 100644 --- a/src/server.rs +++ b/src/server.rs @@ -611,7 +611,15 @@ pub async fn start_server(is_server: bool, no_server: bool) { // so a Wayland host that came up slowly skipped the warm for the life of the process and // got back the cold-probe "No displays" symptom the warm exists to remove. #[cfg(all(target_os = "linux", feature = "drm"))] - std::thread::spawn(drm_capturer::warm_availability); + if let Err(err) = std::thread::Builder::new() + .name("drm-warm".into()) + .spawn(drm_capturer::warm_availability) + { + // Same reason as the root service's startup threads: `thread::spawn` panics on EAGAIN + // and that would abort `start_server`. Skipping the warm costs the first session the + // cold probe, which is what happened before the warm existed. + log::warn!("drm: could not spawn the availability warm ({err}); skipping it"); + } input_service::fix_key_down_timeout_loop(); #[cfg(target_os = "linux")] if input_service::wayland_use_uinput() { diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 5922fbc2f..9ac9a944e 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -337,7 +337,12 @@ impl IpcDrmCapturer { { let shared = shared.clone(); let stop = stop.clone(); - std::thread::spawn(move || recv_thread(display, expected, shared, stop, tx)); + // Builder, like the startup threads: `thread::spawn` panics on EAGAIN, and this runs + // inside a function that already returns ResultType, so a failure has a clean home. + std::thread::Builder::new() + .name("drm-recv".into()) + .spawn(move || recv_thread(display, expected, shared, stop, tx)) + .map_err(|err| anyhow!("could not spawn the drm receive thread: {err}"))?; } let (displays, wire_idx) = match rx.recv_timeout(Duration::from_millis(HANDSHAKE_WAIT_MS)) { Ok(res) => res?, @@ -737,10 +742,14 @@ async fn recv_thread( format: desc.format, modifier: desc.modifier, fb_id: desc.fb_id, - // Clamped although the producer already normalizes it and must be root: this - // value indexes offsets/pitches inside libdrmtap, and the wire is the one place - // it arrives from another process. - num_planes: desc.num_planes.clamp(1, 4), + // Passed through RAW on purpose. This used to clamp to 1..=4, which was added + // before `drm_render::convert` grew its own check -- and convert REJECTS an + // out-of-range count rather than clamping, precisely so the count the C reads is + // the count this side validated. Clamping here made that reject unreachable for + // any over-range wire value: a descriptor claiming 7 planes arrived at convert + // as 4 and passed. One validation site for this field, and it is the one next to + // the code that dereferences it. + num_planes: desc.num_planes, offsets: desc.offsets, pitches: desc.pitches, hdr_eotf: desc.hdr_eotf,