fix(vdisplay): the env lock covered its writers and not its readers, and the gamescope ladder read back its own output

Two halves of the same problem — process env used as shared mutable state.

`apply_session_env` set XDG_RUNTIME_DIR / DBUS_SESSION_BUS_ADDRESS / WAYLAND_DISPLAY /
HYPRLAND_INSTANCE_SIGNATURE / SWAYSOCK under ENV_LOCK, while detection read those same
five keys with no lock at all, from five points spread across a `/proc` scan. That is
exactly the hazard ENV_LOCK's own doc names: glibc `setenv` can realloc `environ` and
free the old value string, so a concurrent `getenv` reads freed memory — UB documented
there as "could crash the host". It is reachable today: the host's session watcher
calls `detect_active_session` every second for a stream's lifetime while a second
client's connect runs `apply_session_env` on a spawn_blocking thread.

Detection now samples all five into an `EnvProbe` in ONE locked read and scans from
that snapshot. Taking the lock around the whole of `detect_active_session` would have
been simpler and wrong: it walks `/proc` every second, and holding a process-wide lock
across a directory walk trades this problem for the one Phase 7 is about. The readers
become pure functions of their inputs, which is also what let the tests stop mutating
process env to steer them — `without_inherited_swaysock` is gone, replaced by handing
in a probe. Empty values are now treated as absent, so `XDG_RUNTIME_DIR=""` falls back
to /run/user/<uid> instead of yielding a relative path.

The gamescope sub-mode ladder had a nastier version: `apply_input_env` WRITES
PUNKTFUNK_GAMESCOPE_NODE/_SESSION to publish the sub-mode it chose, and read the same
keys back as operator overrides. The Attach arm sets `_NODE=auto`, and `node_env` sits
at rung 2 of the ladder — above `dedicated_launch` at rung 3 — so one Attach decision
latched Attach for the rest of the host's life and silently overrode
`game_session=dedicated`. Only rung 1 could escape it, because the Spawn arm that
would have cleared the keys sits below the rung that by then always fired. The
overrides are now sampled once, before this module has written anything, which keeps
their actual meaning: "the operator set this before we ran". The live reads that
remain (`launch_is_nested`, `poolable_now`) are deliberate — those consume the
published decision.

And gamescope's `create` read both keys unguarded, racing the writer and skipping the
lock its two siblings take for the same keys; it now takes one guarded snapshot.

Verified on Linux (.25): 100 passed, 1 ignored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs

@@ -294,13 +294,23 @@ impl VirtualDisplay for GamescopeDisplay {
         // full Steam-Deck-UI session headless at the client's resolution + refresh — so games SEE
         // them (via the injected --nested-refresh + generated CVT modes, not the box's TV EDID) —
         // and relaunch it when the client's mode changes. Reuses the node + EIS discovery below.
-        if let Ok(client) = std::env::var("PUNKTFUNK_GAMESCOPE_SESSION") {
+        // Both keys read in ONE guarded snapshot: `apply_input_env` writes them under the env
+        // lock but releases it before they are consumed, so reading them live here raced its
+        // writes — and read them WITHOUT the lock that `poolable_now` and `launch_is_nested` take
+        // for the same two keys.
+        let (session_env, node_env) = crate::with_env_lock(|| {
+            (
+                std::env::var("PUNKTFUNK_GAMESCOPE_SESSION").ok(),
+                std::env::var("PUNKTFUNK_GAMESCOPE_NODE").ok(),
+            )
+        });
+        if let Some(client) = session_env {
             return create_managed_session(&client, mode, self.hdr);
         }
         // Attach to an already-running gamescope (a foreign / externally-launched session) instead
         // of spawning our own: capture its node AND inject into its EIS socket.
         // PUNKTFUNK_GAMESCOPE_NODE=<id|auto>; "auto" discovers the gamescope `Video/Source` node.
-        if let Ok(id) = std::env::var("PUNKTFUNK_GAMESCOPE_NODE") {
+        if let Some(id) = node_env {
             let node_id: u32 = if id.trim().eq_ignore_ascii_case("auto") {
                 // Attach to the box-owned game-mode session, but FIRST make it run at the connecting
                 // client's resolution (the box is headless, so its game-mode mode is ours to set).

crates/pf-vdisplay/src/vdisplay/routing.rs

@@ -62,8 +62,56 @@ fn pick_gamescope_mode(
 /// foreign gamescope on an infra-less box), or **bare spawn** (a per-session headless gamescope
 /// nesting the session's launch command — the plain-distro default). `PUNKTFUNK_GAMESCOPE_MANAGED`
 /// forces managed over all of it.
+/// The operator's gamescope overrides, sampled ONCE — before this module has written anything.
+///
+/// [`apply_input_env`] both WRITES `PUNKTFUNK_GAMESCOPE_NODE`/`_SESSION` (to publish the sub-mode it
+/// chose) and READS them as operator overrides. Reading them live therefore fed the ladder its own
+/// previous output: the Attach arm sets `_NODE=auto`, and `node_env` sits at rung 2 of
+/// [`pick_gamescope_mode`] — ABOVE `dedicated_launch` at rung 3 — so one Attach decision latched
+/// Attach for the rest of the host's life and silently overrode `game_session=dedicated`. Only rung
+/// 1 (`_MANAGED`) could escape, because the Spawn arm that would clear the keys sits below the rung
+/// that by then always fired.
+///
+/// Sampling at first use keeps the override's actual meaning — "the operator set this before we
+/// ran" — and makes it immune to our own writes. The live reads that remain
+/// ([`launch_is_nested`], gamescope's `poolable_now`) are deliberate: those consume the PUBLISHED
+/// decision, which is what the keys carry after this function has run.
+#[cfg(target_os = "linux")]
+static OPERATOR_GAMESCOPE: std::sync::OnceLock<OperatorGamescope> = std::sync::OnceLock::new();
+
+#[cfg(target_os = "linux")]
+#[derive(Clone, Copy, Debug)]
+struct OperatorGamescope {
+    managed: bool,
+    attach: bool,
+    node: bool,
+    session: bool,
+}
+
+#[cfg(target_os = "linux")]
+fn operator_gamescope() -> OperatorGamescope {
+    *OPERATOR_GAMESCOPE.get_or_init(|| {
+        let ov = with_env_lock(|| OperatorGamescope {
+            managed: std::env::var_os("PUNKTFUNK_GAMESCOPE_MANAGED").is_some(),
+            attach: std::env::var_os("PUNKTFUNK_GAMESCOPE_ATTACH").is_some(),
+            node: std::env::var_os("PUNKTFUNK_GAMESCOPE_NODE").is_some(),
+            session: std::env::var_os("PUNKTFUNK_GAMESCOPE_SESSION").is_some(),
+        });
+        if ov.managed || ov.attach || ov.node || ov.session {
+            tracing::info!(
+                ?ov,
+                "gamescope: operator sub-mode overrides sampled from the environment"
+            );
+        }
+        ov
+    })
+}
+
 #[cfg(target_os = "linux")]
 pub fn apply_input_env(chosen: Compositor, dedicated_launch: bool) {
+    // Sampled BEFORE the lock — `operator_gamescope` takes it itself, and this mutex is not
+    // reentrant.
+    let ov = operator_gamescope();
     let _env_guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());
     let backend = match chosen {
         Compositor::Gamescope => "gamescope",
@@ -80,10 +128,10 @@ pub fn apply_input_env(chosen: Compositor, dedicated_launch: bool) {
     if chosen == Compositor::Gamescope {
         let mode = pick_gamescope_mode(
             dedicated_launch,
-            std::env::var_os("PUNKTFUNK_GAMESCOPE_MANAGED").is_some(),
-            std::env::var_os("PUNKTFUNK_GAMESCOPE_ATTACH").is_some(),
-            std::env::var_os("PUNKTFUNK_GAMESCOPE_NODE").is_some(),
-            std::env::var_os("PUNKTFUNK_GAMESCOPE_SESSION").is_some(),
+            ov.managed,
+            ov.attach,
+            ov.node,
+            ov.session,
             gamescope::managed_session_available(),
             gamescope::foreign_gamescope_running(),
         );
@@ -316,4 +364,27 @@ mod tests {
         assert_eq!(pick(true, false, true, false, false, false, false), Attach);
         assert_eq!(pick(true, false, false, true, false, false, false), Attach);
     }
+
+    /// The ladder must not be able to read back its own output. `apply_input_env`'s Attach arm
+    /// writes `PUNKTFUNK_GAMESCOPE_NODE=auto`, and `node_env` outranks `dedicated_launch` — so when
+    /// the override was read live, one Attach latched Attach for the host's lifetime and silently
+    /// overrode `game_session=dedicated`. Sampling once is what breaks the loop; this pins that the
+    /// sample does not move when the key is written afterwards.
+    #[test]
+    #[cfg(target_os = "linux")]
+    fn operator_overrides_do_not_see_our_own_writes() {
+        use super::operator_gamescope;
+        let first = operator_gamescope();
+        let restore = crate::with_env_lock(|| std::env::var_os("PUNKTFUNK_GAMESCOPE_NODE"));
+        crate::with_env_lock(|| std::env::set_var("PUNKTFUNK_GAMESCOPE_NODE", "auto"));
+        let second = operator_gamescope();
+        crate::with_env_lock(|| match &restore {
+            Some(v) => std::env::set_var("PUNKTFUNK_GAMESCOPE_NODE", v),
+            None => std::env::remove_var("PUNKTFUNK_GAMESCOPE_NODE"),
+        });
+        assert_eq!(
+            second.node, first.node,
+            "writing the key we publish must not turn into an operator override"
+        );
+    }
 }

crates/pf-vdisplay/src/vdisplay/session.rs

@@ -190,11 +190,12 @@ impl ActiveSession {
     // session to describe.
     #[cfg_attr(target_os = "linux", allow(dead_code))]
     fn none() -> ActiveSession {
+        let probe = EnvProbe::sample();
         ActiveSession {
             kind: ActiveKind::None,
             env: SessionEnv {
-                xdg_runtime_dir: default_runtime_dir(),
-                dbus_session_bus_address: default_bus(&default_runtime_dir()),
+                xdg_runtime_dir: default_runtime_dir(&probe),
+                dbus_session_bus_address: default_bus(&probe, &default_runtime_dir(&probe)),
                 ..Default::default()
             },
             compositor_pid: None,
@@ -214,9 +215,49 @@ pub fn compositor_for_kind(kind: ActiveKind) -> Option<Compositor> {
     }
 }
 
+/// The session-scoped variables detection reads, sampled ONCE under [`ENV_LOCK`].
+///
+/// Detection used to call `std::env::var` at five points spread across a `/proc` scan, none of them
+/// holding the lock its own writers take — the getenv/setenv data race [`crate::ENV_LOCK`]'s doc
+/// describes as UB that "could crash the host" (glibc `setenv` can realloc `environ` and free the
+/// old value string under a concurrent reader). Sampling up front closes that.
+///
+/// Sampling rather than simply taking the lock for the whole of [`detect_active_session`] is
+/// deliberate: that function runs every second from the host's session watcher and scans `/proc`,
+/// and holding a process-wide lock across a directory walk trades one problem for another. The
+/// snapshot costs one acquisition and five reads with no syscalls in between.
+///
+/// It also makes the readers below pure functions of their inputs, which is what lets the tests
+/// exercise them without mutating process-global state.
+#[derive(Clone, Debug, Default)]
+struct EnvProbe {
+    xdg_runtime_dir: Option<String>,
+    dbus_session_bus_address: Option<String>,
+    wayland_display: Option<String>,
+    hyprland_signature: Option<String>,
+    swaysock: Option<String>,
+}
+
+impl EnvProbe {
+    /// Every var is `filter`ed non-empty: `Ok("")` is not a usable runtime dir or socket path, and
+    /// treating it as one is how an empty `XDG_RUNTIME_DIR` used to yield a *relative* path.
+    fn sample() -> EnvProbe {
+        fn v(k: &str) -> Option<String> {
+            std::env::var(k).ok().filter(|s| !s.is_empty())
+        }
+        crate::with_env_lock(|| EnvProbe {
+            xdg_runtime_dir: v("XDG_RUNTIME_DIR"),
+            dbus_session_bus_address: v("DBUS_SESSION_BUS_ADDRESS"),
+            wayland_display: v("WAYLAND_DISPLAY"),
+            hyprland_signature: v("HYPRLAND_INSTANCE_SIGNATURE"),
+            swaysock: v("SWAYSOCK"),
+        })
+    }
+}
+
 #[cfg(target_os = "linux")]
-fn default_runtime_dir() -> String {
-    std::env::var("XDG_RUNTIME_DIR").unwrap_or_else(|_| {
+fn default_runtime_dir(env: &EnvProbe) -> String {
+    env.xdg_runtime_dir.clone().unwrap_or_else(|| {
         // SAFETY: `getuid()` is a parameterless POSIX call that always succeeds and touches no
         // memory — it just returns the calling process's real uid. Nothing is aliased or freed.
         let uid = unsafe { libc::getuid() };
@@ -225,12 +266,14 @@ fn default_runtime_dir() -> String {
 }
 
 #[cfg(not(target_os = "linux"))]
-fn default_runtime_dir() -> String {
-    std::env::var("XDG_RUNTIME_DIR").unwrap_or_default()
+fn default_runtime_dir(env: &EnvProbe) -> String {
+    env.xdg_runtime_dir.clone().unwrap_or_default()
 }
 
-fn default_bus(runtime: &str) -> String {
-    std::env::var("DBUS_SESSION_BUS_ADDRESS").unwrap_or_else(|_| format!("unix:path={runtime}/bus"))
+fn default_bus(env: &EnvProbe, runtime: &str) -> String {
+    env.dbus_session_bus_address
+        .clone()
+        .unwrap_or_else(|| format!("unix:path={runtime}/bus"))
 }
 
 /// Detect the graphical session live for our uid right now (cheap, side-effect-free: a `/proc`
@@ -243,8 +286,11 @@ pub fn detect_active_session() -> ActiveSession {
     // SAFETY: `getuid()` is a parameterless POSIX call that always succeeds and touches no memory —
     // it just returns the calling process's real uid. Nothing is aliased or freed.
     let uid = unsafe { libc::getuid() };
-    let xdg_runtime_dir = default_runtime_dir();
-    let dbus = default_bus(&xdg_runtime_dir);
+    // ONE sample of the session-scoped env, before any scanning — see [`EnvProbe`]. Everything
+    // below reads this snapshot, never the process env.
+    let env = EnvProbe::sample();
+    let xdg_runtime_dir = default_runtime_dir(&env);
+    let dbus = default_bus(&env, &xdg_runtime_dir);
 
     // Process probe: the running graphical compositor of THIS uid decides the kind. Priority lets
     // a real desktop (kwin/gnome/sway) win over a leftover gamescope child. comm names mirror the
@@ -306,7 +352,7 @@ pub fn detect_active_session() -> ActiveSession {
     // driven and don't.
     let wayland_display = match kind {
         ActiveKind::DesktopKde | ActiveKind::DesktopWlroots | ActiveKind::DesktopHyprland => {
-            find_wayland_socket(&xdg_runtime_dir, uid)
+            find_wayland_socket(&env, &xdg_runtime_dir, uid)
         }
         _ => None,
     };
@@ -323,13 +369,13 @@ pub fn detect_active_session() -> ActiveSession {
     // Discover the Hyprland instance signature so `hyprctl` can reach the compositor even when the
     // host runs as a systemd `--user` service that never inherited the session env.
     let hyprland_signature = match kind {
-        ActiveKind::DesktopHyprland => find_hypr_signature(&xdg_runtime_dir, uid),
+        ActiveKind::DesktopHyprland => find_hypr_signature(&env, &xdg_runtime_dir, uid),
         _ => None,
     };
     // Same idea for sway's IPC socket: without it `swaymsg` has nothing to talk to, and a
     // `systemd --user` host never inherited it.
     let sway_socket = match kind {
-        ActiveKind::DesktopWlroots => find_sway_socket(&xdg_runtime_dir, uid, winning_pid),
+        ActiveKind::DesktopWlroots => find_sway_socket(&env, &xdg_runtime_dir, uid, winning_pid),
         _ => None,
     };
     ActiveSession {
@@ -353,12 +399,12 @@ pub fn detect_active_session() -> ActiveSession {
 /// normally exposes exactly one. (Phase-2 refinement: match the instance to `compositor_pid` via
 /// `hyprctl instances` when several coexist — `design/hyprland-support.md` §Phase-1.1.)
 #[cfg(target_os = "linux")]
-fn find_hypr_signature(runtime: &str, uid: u32) -> Option<String> {
+fn find_hypr_signature(env: &EnvProbe, runtime: &str, uid: u32) -> Option<String> {
     use std::os::unix::fs::MetadataExt;
     let hypr = std::path::Path::new(runtime).join("hypr");
-    if let Ok(sig) = std::env::var("HYPRLAND_INSTANCE_SIGNATURE") {
-        if !sig.is_empty() && hypr.join(&sig).join(".socket.sock").exists() {
-            return Some(sig);
+    if let Some(sig) = &env.hyprland_signature {
+        if hypr.join(sig).join(".socket.sock").exists() {
+            return Some(sig.clone());
         }
     }
     let mut cands: Vec<(std::time::SystemTime, String)> = Vec::new();
@@ -387,11 +433,11 @@ fn find_hypr_signature(runtime: &str, uid: u32) -> Option<String> {
 /// `None` on river: it is the other [`ActiveKind::DesktopWlroots`] compositor and has no sway IPC —
 /// which is the honest answer, since the wlroots backend drives sway through `swaymsg`.
 #[cfg(target_os = "linux")]
-fn find_sway_socket(runtime: &str, uid: u32, pid: Option<u32>) -> Option<String> {
+fn find_sway_socket(env: &EnvProbe, runtime: &str, uid: u32, pid: Option<u32>) -> Option<String> {
     use std::os::unix::fs::MetadataExt;
-    if let Ok(s) = std::env::var("SWAYSOCK") {
-        if !s.is_empty() && std::path::Path::new(&s).exists() {
-            return Some(s);
+    if let Some(s) = &env.swaysock {
+        if std::path::Path::new(s).exists() {
+            return Some(s.clone());
         }
     }
     if let Some(pid) = pid {
@@ -427,10 +473,10 @@ pub fn detect_active_session() -> ActiveSession {
 /// valid inherited `WAYLAND_DISPLAY` first; otherwise take the newest-mtime socket we own (a
 /// desktop session normally exposes exactly one).
 #[cfg(target_os = "linux")]
-fn find_wayland_socket(runtime: &str, uid: u32) -> Option<String> {
+fn find_wayland_socket(env: &EnvProbe, runtime: &str, uid: u32) -> Option<String> {
     use std::os::unix::fs::MetadataExt;
-    if let Ok(w) = std::env::var("WAYLAND_DISPLAY") {
-        if !w.is_empty() {
+    if let Some(w) = env.wayland_display.clone() {
+        {
             let p = if w.starts_with('/') {
                 std::path::PathBuf::from(&w)
             } else {
@@ -666,18 +712,11 @@ mod tests {
         }
     }
 
-    /// Run `f` with `SWAYSOCK` unset, so the "trust what we inherited" rung can't decide the test.
-    /// Serialized on the crate's env lock — these tests mutate process-global state.
-    fn without_inherited_swaysock<R>(f: impl FnOnce() -> R) -> R {
-        crate::with_env_lock(|| {
-            let prev = std::env::var_os("SWAYSOCK");
-            std::env::remove_var("SWAYSOCK");
-            let out = f();
-            if let Some(p) = prev {
-                std::env::set_var("SWAYSOCK", p);
-            }
-            out
-        })
+    /// A probe with nothing inherited, so the "trust what we inherited" rung can't decide the test.
+    /// The readers take this by argument, so — unlike the `set_var`/`remove_var` dance this replaced
+    /// — the tests no longer mutate process-global state to steer them.
+    fn no_inherited_env() -> EnvProbe {
+        EnvProbe::default()
     }
 
     /// The point of deriving it: the socket that belongs to the compositor detection actually found,
@@ -686,7 +725,7 @@ mod tests {
     #[test]
     fn the_socket_matching_the_detected_pid_wins() {
         let rt = FakeRuntime::new("exact", &[4242, 9999]);
-        let got = without_inherited_swaysock(|| find_sway_socket(rt.path(), rt.uid, Some(4242)));
+        let got = find_sway_socket(&no_inherited_env(), rt.path(), rt.uid, Some(4242));
         assert_eq!(
             got,
             Some(format!("{}/sway-ipc.{}.4242.sock", rt.path(), rt.uid))
@@ -698,7 +737,7 @@ mod tests {
     #[test]
     fn an_unmatched_pid_falls_back_to_the_socket_that_is_there() {
         let rt = FakeRuntime::new("fallback", &[777]);
-        let got = without_inherited_swaysock(|| find_sway_socket(rt.path(), rt.uid, Some(12345)));
+        let got = find_sway_socket(&no_inherited_env(), rt.path(), rt.uid, Some(12345));
         assert_eq!(
             got,
             Some(format!("{}/sway-ipc.{}.777.sock", rt.path(), rt.uid))
@@ -711,7 +750,7 @@ mod tests {
     #[test]
     fn no_sway_ipc_socket_reports_none() {
         let rt = FakeRuntime::new("none", &[]);
-        let got = without_inherited_swaysock(|| find_sway_socket(rt.path(), rt.uid, Some(1)));
+        let got = find_sway_socket(&no_inherited_env(), rt.path(), rt.uid, Some(1));
         assert_eq!(got, None);
     }
 
@@ -725,7 +764,7 @@ mod tests {
         let rt = FakeRuntime::new("otheruid", &[]);
         let other = rt.uid.wrapping_add(1);
         std::fs::write(rt.dir.join(format!("sway-ipc.{other}.500.sock")), b"").unwrap();
-        let got = without_inherited_swaysock(|| find_sway_socket(rt.path(), rt.uid, Some(500)));
+        let got = find_sway_socket(&no_inherited_env(), rt.path(), rt.uid, Some(500));
         assert_eq!(got, None);
     }
 
@@ -757,7 +796,7 @@ mod tests {
         // Query with a pid that does NOT match the filename: the exact-path shortcut earlier in
         // `find_sway_socket` returns before the ownership guard, so hitting it would make this
         // test vacuous in the same way the one above was.
-        let got = without_inherited_swaysock(|| find_sway_socket(rt.path(), rt.uid, Some(999)));
+        let got = find_sway_socket(&no_inherited_env(), rt.path(), rt.uid, Some(999));
         assert_eq!(got, None, "a socket owned by another uid must be rejected");
     }
 }