From 972af2992f1b67015f1cdea0a4add1d7aa069f59 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 13:54:56 +0200 Subject: [PATCH] fix(pf-capture): the gamescope cursor fallback rewrote environ under a live multithreaded host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `connect_via_env_swap` did set_var("XAUTHORITY", …) / connect / restore, guarded by a mutex that serialised this source against itself and against nothing else. `getenv` takes no lock. setenv/unsetenv rewrite the process-global `environ`, and glibc REALLOCATES that array when a variable is added — while, at that exact moment, the PipeWire thread is inside pw_init()'s dlopen making bare getenv() calls and EGL/CUDA init is running alongside. The file's own doc already called the pattern "unsound from a live multithreaded host"; it stayed as a fallback. Three things made it worse than the comment suggested: - The damaging branch is the one where XAUTHORITY is ABSENT and therefore gets ADDED (the realloc case). scripts/punktfunk-host.service deliberately does not import the login shell's environment, so absent is the DOCUMENTED NORMAL configuration for the shipped unit, not an edge case. - `rediscover` re-runs this every 2 s for the whole session. A display whose connect fails is never pushed into `displays`, so the dead-display skip never covers it — the race is not once at startup, it repeats forever. - It is unfixable in place. Sharing pf_vdisplay's ENV_LOCK is the wrong layer: it cannot make C `getenv` take a lock. The fix is to stop writing `environ` at all. Connecting with an explicitly empty auth token is what the swap actually achieved: we only reach the fallback when our own lookup found no usable MIT-MAGIC-COOKIE-1 entry, and x11rb's internal lookup reads the same file with a STRICTER matcher (it matches family/address too, which we deliberately do not), so where we find nothing it finds nothing either and connects unauthenticated. That is exactly why the swap "worked" against a nested Xwayland started without -auth. Gives up one case: an .Xauthority using an auth family we decline to guess at but x11rb would have handled. A gamescope Xwayland writes a single-entry MIT-MAGIC-COOKIE-1 file, so it is not reachable here, and declining to attach a cursor overlay beats tearing `environ` out from under a live session. Also removes XAUTH_LOCK, whose only user this was. Verified on 192.168.1.25 (Ubuntu, cargo 1.96.0 — the pinned toolchain, pipewire dev headers present): `cargo check -p pf-capture --locked` and `cargo clippy -p pf-capture --all-targets --locked -- -D warnings` both clean. Not verified on glass: the fallback is only reached when the cookie parse fails, so a normal gamescope session does not enter it. Forcing it needs a nested Xwayland started without -auth, or a mangled cookie file, on .181/.136. --- crates/pf-capture/src/linux/xfixes_cursor.rs | 73 ++++++++++---------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/crates/pf-capture/src/linux/xfixes_cursor.rs b/crates/pf-capture/src/linux/xfixes_cursor.rs index fcea2440..41fd0721 100644 --- a/crates/pf-capture/src/linux/xfixes_cursor.rs +++ b/crates/pf-capture/src/linux/xfixes_cursor.rs @@ -55,16 +55,6 @@ use x11rb::rust_connection::{DefaultStream, RustConnection}; use crate::GamescopeCursorTargets; -/// Serializes the `XAUTHORITY` env swap of the LEGACY connect fallback (the var is process-global). -/// -/// The fallback is a last resort now — see [`connect_conn`]. It serialises this source against -/// itself and nothing else: `getenv` needs no lock to be racy, so every OTHER thread's read (libspa -/// plugin load, EGL/CUDA init — concurrent by construction, since `attach_gamescope_cursor` runs -/// while the PipeWire thread is starting) could still observe the swapped value or a torn -/// environ. That is why the primary path parses the cookie itself and never touches the -/// environment. -static XAUTH_LOCK: Mutex<()> = Mutex::new(()); - /// The `MIT-MAGIC-COOKIE-1` auth-protocol name, as it appears in an `.Xauthority` entry. const MIT_MAGIC_COOKIE_1: &[u8] = b"MIT-MAGIC-COOKIE-1"; @@ -267,17 +257,18 @@ fn connect(dpy: &str, xauthority: Option<&str>) -> Result { /// environment. /// /// `RustConnection::connect` reads `XAUTHORITY` from the env, so the original implementation -/// `set_var`'d it around each connect under [`XAUTH_LOCK`]. That is unsound from a live -/// multithreaded host: the lock serialises this source against itself, but `getenv` takes no lock, -/// so any concurrent reader (libspa's plugin load, EGL/CUDA init — running at exactly this moment, -/// since the PipeWire thread is starting up) could read the swapped value or race the environ -/// rewrite outright. The project already has a process-wide env-lock discipline elsewhere, but -/// sharing it would be the wrong layer AND would still not fix `getenv`. +/// `set_var`'d it around each connect under a mutex. That is unsound from a live multithreaded +/// host: the lock serialised this source against itself, but `getenv` takes no lock, so any +/// concurrent reader (libspa's plugin load, EGL/CUDA init — running at exactly this moment, since +/// the PipeWire thread is starting up) could read the swapped value or race the environ rewrite +/// outright. The project already has a process-wide env-lock discipline elsewhere, but sharing it +/// would be the wrong layer AND would still not fix `getenv`. /// /// So: parse the MIT-MAGIC-COOKIE-1 entry out of the file ourselves and hand it to /// `connect_to_stream_with_auth_info`, which is what `RustConnection::connect` does internally with -/// the cookie IT found. The env swap survives only as a fallback for a file we cannot parse (an -/// unexpected layout, or an auth family whose entry we decline to guess at). +/// the cookie IT found. Where that finds nothing usable we connect with an explicitly empty token +/// ([`connect_unauthenticated`]) rather than swapping the environment — this process no longer +/// writes `environ` at all. fn connect_conn(dpy: &str, xauthority: Option<&str>) -> Result<(RustConnection, usize), String> { let Some(path) = xauthority else { // No per-display cookie file to inject: the ambient environment is already what this @@ -289,16 +280,16 @@ fn connect_conn(dpy: &str, xauthority: Option<&str>) -> Result<(RustConnection, Ok(v) => return Ok(v), Err(e) => tracing::debug!( dpy = %dpy, xauthority = %path, error = %e, - "gamescope cursor: cookie connect failed — falling back to the XAUTHORITY env swap" + "gamescope cursor: cookie connect failed — retrying unauthenticated" ), }, None => tracing::debug!( dpy = %dpy, xauthority = %path, - "gamescope cursor: no MIT-MAGIC-COOKIE-1 entry for this display — falling back to the \ - XAUTHORITY env swap" + "gamescope cursor: no MIT-MAGIC-COOKIE-1 entry for this display — connecting \ + unauthenticated" ), } - connect_via_env_swap(dpy, path) + connect_unauthenticated(dpy) } /// Connect to `dpy` and complete the setup handshake with an explicit cookie — the same two steps @@ -331,19 +322,31 @@ fn connect_with_cookie( .map_err(|e| format!("setup: {e}")) } -/// LEGACY fallback (see [`connect_conn`]): swap `XAUTHORITY`, connect, restore. Serialised against -/// this source's own concurrent connects, but NOT against other threads' `getenv` — which is why it -/// is a fallback and not the path taken. -fn connect_via_env_swap(dpy: &str, xauthority: &str) -> Result<(RustConnection, usize), String> { - let _g = XAUTH_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let prev = std::env::var_os("XAUTHORITY"); - std::env::set_var("XAUTHORITY", xauthority); - let out = RustConnection::connect(Some(dpy)); - match prev { - Some(p) => std::env::set_var("XAUTHORITY", p), - None => std::env::remove_var("XAUTHORITY"), - } - out.map_err(|e| format!("connect: {e}")) +/// Last-resort fallback (see [`connect_conn`]): connect with an EXPLICITLY EMPTY auth token. +/// +/// This replaces a `set_var("XAUTHORITY", …)` / connect / restore dance, which was unsound and is +/// not fixable in place. `setenv`/`unsetenv` rewrite the process-global `environ`; glibc +/// *reallocates* that array when a variable is added, and the host is emphatically multithreaded +/// at this moment — `attach_gamescope_cursor` runs while the PipeWire thread is inside `pw_init`'s +/// `dlopen` and a dozen bare `getenv()` calls, with EGL/CUDA init alongside. A mutex here +/// serialised this source against itself and against nothing else, because `getenv` takes no lock. +/// The damaging branch is the one where `XAUTHORITY` is ABSENT and therefore gets *added* — which +/// `scripts/punktfunk-host.service` makes the normal configuration, since the unit deliberately +/// does not import the login shell's environment. And `rediscover` re-runs this every 2 s for the +/// whole session, because a display whose connect fails is never recorded and so is never skipped. +/// +/// Connecting with an empty token is what the swap actually achieved. We only reach here when our +/// own lookup found no usable `MIT-MAGIC-COOKIE-1` entry, and x11rb's internal lookup reads the +/// same file with a STRICTER matcher (it also matches family/address, which we deliberately do +/// not) — so where we find nothing, it finds nothing too, and connects unauthenticated. That is +/// precisely why the swap "worked" against a nested Xwayland started without `-auth`. +/// +/// The one case this gives up is an `.Xauthority` whose entry uses an auth family we decline to +/// guess at but x11rb would have handled. A gamescope Xwayland writes a single-entry +/// MIT-MAGIC-COOKIE-1 file, so that case is not reachable here — and a cursor overlay that +/// declines to attach is the correct outcome anyway, against a torn `environ` in a live session. +fn connect_unauthenticated(dpy: &str) -> Result<(RustConnection, usize), String> { + connect_with_cookie(dpy, Vec::new(), Vec::new()) } /// The `MIT-MAGIC-COOKIE-1` `(name, data)` for `dpy` from the `.Xauthority`-format file at `path`.