From 002702bcec1cd2fb88b862a3ee80d72b94638c2c Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 10 Aug 2026 18:38:14 +0200 Subject: [PATCH] =?UTF-8?q?fix(pf-vdisplay):=20NixOS=20sessions=20were=20u?= =?UTF-8?q?ndetectable=20=E2=80=94=20comm=20is=20the=20WRAPPER's=20name?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session probe decided "is a desktop live?" by reading /proc//comm for every process of our uid and exact-matching it against "kwin_wayland" / "gamescope" / "gnome-shell" / "Hyprland". comm is the kernel's name for the executed FILE, truncated to 15 bytes — not argv[0]. nixpkgs wraps essentially every graphical binary: wrapProgram moves the real ELF aside to `.-wrapped` and installs a wrapper under the original name, which then `exec -a "$0"`s the hidden file. So on NixOS the kernel reports `.kwin_wayland-w` (15 bytes of `.kwin_wayland-wrapped`) while ps/pgrep -a show a perfectly ordinary `kwin_wayland`, because they read argv. Measured against a live kernel: `.kwin_wayland-w`, `.kwin_wayland_w` (KWin's own kwin_wayland_wrapper), `.gamescope-wrap`, all 15 bytes. Nothing downstream could recover from that one string comparison: - detect_active_session returned ActiveKind::None on a *running* KDE desktop; - wayland_display is only resolved for a detected kind, so the connect log reported wayland="-" even though WAYLAND_DISPLAY was correct; - pick_compositor's Auto arm returns the DETECTED backend, so a live, fully working KWin sitting in available() was never chosen — every connect died "no usable compositor"; - and PUNKTFUNK_COMPOSITOR could not rescue it: pinned_at_a_dead_session consults the same probe, turning the miss into a hard error instead. No environment variable reached the comparison — the XDG_CURRENT_DESKTOP fallback in detect() is only on the pinned path. Capture itself was never at fault: a decoy process merely NAMED kwin_wayland satisfied the probe and the stream came up against the real KWin. Resolve the name through /proc//exe (the full, untruncated file name) and strip the nixpkgs decoration. Both the leading `.` and the trailing `-wrapped` are required before anything is stripped, so KWin's own real `kwin_wayland_wrapper` binary keeps its name rather than collapsing into `kwin_wayland` and handing the probe the parent's PID. The comm fast path is kept for every ordinary distro — one read, no readlink, and no name that matched before can stop matching. Also applied to foreign_gamescope_running, which had the same defect: nixpkgs wraps gamescope too, so the attach-vs-spawn ladder saw no foreign session. Tests are fixture-driven rather than spawn-driven on purpose: a stand-in has to be a real ELF that tolerates being renamed, and /bin/sleep is not one — modern coreutils is a multi-call binary that dispatches on the executable's own name, so a copy called `.kwin_wayland-wrapped` exits instantly and /proc//exe is gone before it can be read. That failure looks exactly like this resolver being broken; it cost one debugging round here and the same trap is already recorded in punktfunk-host's /proc matcher. --- .../src/vdisplay/linux/gamescope.rs | 6 +- crates/pf-vdisplay/src/vdisplay/proc.rs | 258 ++++++++++++++++++ crates/pf-vdisplay/src/vdisplay/session.rs | 11 +- 3 files changed, 269 insertions(+), 6 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs b/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs index 6dcba18f..b7a06251 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs @@ -645,10 +645,12 @@ pub fn foreign_gamescope_running() -> bool { if md.uid() != uid { continue; } - let Ok(comm) = std::fs::read_to_string(e.path().join("comm")) else { + // Resolved, not a raw `comm` read: nixpkgs wraps gamescope too, so on NixOS the kernel + // reports `.gamescope-wrap` and this probe saw no foreign session at all. + let Some(comm) = crate::proc::match_name(&e.path()) else { continue; }; - if !matches!(comm.trim(), "gamescope" | "gamescope-wl") { + if !matches!(comm.as_str(), "gamescope" | "gamescope-wl") { continue; } if !descends_from(pid, our_pid) { diff --git a/crates/pf-vdisplay/src/vdisplay/proc.rs b/crates/pf-vdisplay/src/vdisplay/proc.rs index 0c38b91b..a2971875 100644 --- a/crates/pf-vdisplay/src/vdisplay/proc.rs +++ b/crates/pf-vdisplay/src/vdisplay/proc.rs @@ -111,6 +111,74 @@ pub(crate) fn current_uid() -> u32 { unsafe { libc::getuid() } } +/// The longest `/proc//comm` the kernel will report: `TASK_COMM_LEN` is 16 *including* the +/// NUL, so a name of exactly this many bytes may be a truncation of a longer one. +#[cfg(target_os = "linux")] +const COMM_MAX: usize = 15; + +/// The executable name to identify a process by, with nixpkgs wrapper decoration undone. +/// +/// `comm` is the kernel's name for the **executed file**, truncated to [`COMM_MAX`] bytes — it is +/// not `argv[0]` and not the command line. nixpkgs wraps essentially every graphical binary: +/// `wrapProgram` moves the real ELF aside to `.-wrapped` and installs a shell wrapper under +/// the original name, and that wrapper `exec -a "$0"`s the hidden file. So `ps`/`pgrep -a` show a +/// perfectly ordinary `kwin_wayland` (they read argv) while the kernel reports `.kwin_wayland-w` +/// — 15 bytes of `.kwin_wayland-wrapped`, which can never equal `kwin_wayland`. +/// +/// That is not a KDE-only detail. On NixOS `kwin_wayland`, `gamescope`, `gnome-shell` and +/// `Hyprland` are all wrapped, so an exact `comm` comparison made [`super::session`]'s probe +/// answer [`crate::ActiveKind::None`] on a visibly running desktop — and because the probe is the +/// *only* input to that decision, no environment variable could reach it: `WAYLAND_DISPLAY` was +/// correct, capture worked the moment detection was satisfied, and a `PUNKTFUNK_COMPOSITOR` pin +/// turned the miss into a hard error via `pinned_at_a_dead_session`. (sway survives by accident — +/// nixpkgs' wrapper execs a real binary that is itself still called `sway`.) +/// +/// The `comm` fast path is kept for every ordinary distro: one read, no readlink. Only a name that +/// *could* be decorated or truncated — it starts with `.`, or it is exactly [`COMM_MAX`] bytes — +/// is re-resolved through `/proc//exe`, which carries the full, untruncated file name. +/// +/// `pid_path` is a `/proc/` directory. `None` when the process vanished mid-scan. +#[cfg(target_os = "linux")] +pub(crate) fn match_name(pid_path: &std::path::Path) -> Option { + let comm = std::fs::read_to_string(pid_path.join("comm")).ok()?; + let comm = comm.trim(); + // An undecorated name short enough to be complete is already the answer. + if !comm.starts_with('.') && comm.len() < COMM_MAX { + return Some(comm.to_string()); + } + // Reading our OWN uid's `/proc//exe` needs no privilege (every caller filters on uid + // first), but it is still absent for a kernel thread and for a process exiting under us — + // in which case the truncated `comm` is the best that exists. + match std::fs::read_link(pid_path.join("exe")) + .ok() + .as_deref() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + { + Some(full) => Some(undecorate(full).to_string()), + None => Some(comm.to_string()), + } +} + +/// Strip nixpkgs `wrapProgram` decoration: `.-wrapped`, plus the `_` suffixes make-wrapper +/// appends when that hidden name is already taken (a doubly-wrapped app — Qt *and* GApps). +/// +/// **Both** halves are required, and that is the load-bearing part rather than pedantry: KWin +/// ships its own real binary called `kwin_wayland_wrapper` (the session's parent process), so a +/// rule that merely stripped a `wrapper`-ish suffix would rewrite it into `kwin_wayland` and hand +/// the session probe the wrong PID. Demanding the leading `.` as well keeps it — and any genuine +/// `foo-wrapped` — under its real name. +#[cfg(target_os = "linux")] +fn undecorate(name: &str) -> &str { + let Some(rest) = name.strip_prefix('.') else { + return name; + }; + match rest.trim_end_matches('_').strip_suffix("-wrapped") { + Some(real) if !real.is_empty() => real, + _ => name, + } +} + /// Ending the *tree* the helper started, not just the process we spawned. /// /// [`std::process::Child::kill`] is one `TerminateProcess` / one `SIGKILL`: it ends exactly the @@ -280,6 +348,196 @@ mod tests { } } +/// The `comm`-vs-real-name resolution ([`match_name`]). Linux-only, because the trap it exists for +/// is a Linux kernel detail (`comm` names the executed FILE, truncated to 15 bytes) crossed with a +/// nixpkgs packaging convention. +/// +/// Driven against **fixture** `/proc/` directories rather than spawned processes, for the same +/// reason the `/proc` matcher in `punktfunk-host` learned the hard way: a stand-in has to be a real +/// ELF that tolerates being *renamed*, and `/bin/sleep` is not one. Modern coreutils (uutils on +/// Ubuntu 25.10+, busybox elsewhere) is a MULTI-CALL binary — copied to `.kwin_wayland-wrapped` it +/// prints "unknown program" and exits before `/proc` can be read, and restoring `argv[0]` does not +/// save it. That reads exactly like this resolver being broken. The truncation the fixtures encode +/// is not guessed: the strings below were measured from a live kernel (`.kwin_wayland-w`, +/// `.kwin_wayland_w`, `.gamescope-wrap` — all 15 bytes) against binaries installed and exec'd the +/// way nixpkgs does it. +#[cfg(all(test, target_os = "linux"))] +mod name_tests { + use super::*; + use std::path::{Path, PathBuf}; + + /// A fake `/proc/` directory: a `comm` file and, optionally, the `exe` symlink. Removed on + /// drop. + struct FakePid { + dir: PathBuf, + } + + impl FakePid { + /// `comm` is written exactly as the kernel would report it — i.e. already truncated. + fn new(tag: &str, comm: &str, exe: Option<&str>) -> FakePid { + let dir = std::env::temp_dir().join(format!("pf-vd-name-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("fixture dir"); + std::fs::write(dir.join("comm"), format!("{comm}\n")).expect("comm"); + if let Some(exe) = exe { + // The target need not exist: `read_link` reports the link's contents, and a real + // `/proc//exe` routinely points at a path that has since been replaced. + std::os::unix::fs::symlink( + format!("/nix/store/eeee-kwin-6.5.0/bin/{exe}"), + dir.join("exe"), + ) + .expect("exe symlink"); + } + FakePid { dir } + } + fn path(&self) -> &Path { + &self.dir + } + } + + impl Drop for FakePid { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.dir); + } + } + + /// The decoration table. The `kwin_wayland_wrapper` rows are the ones that earn their keep: it + /// is a REAL KWin binary (the session's parent process), so the rule must leave it under its own + /// name in both its plain and its wrapped form rather than collapsing either into + /// `kwin_wayland` and handing the session probe the wrong PID. + #[test] + fn undecorate_strips_only_a_real_nixpkgs_wrapper() { + for (raw, want) in [ + (".kwin_wayland-wrapped", "kwin_wayland"), + (".gamescope-wrapped", "gamescope"), + (".gnome-shell-wrapped", "gnome-shell"), + (".Hyprland-wrapped", "Hyprland"), + // make-wrapper appends `_`s when the hidden name is already taken (a Qt + GApps + // double-wrap), so the underscores come off before the suffix does. + (".kwin_wayland-wrapped_", "kwin_wayland"), + (".kwin_wayland-wrapped__", "kwin_wayland"), + // Not decoration — every one of these keeps its exact name. + ("kwin_wayland", "kwin_wayland"), + ("kwin_wayland_wrapper", "kwin_wayland_wrapper"), + (".kwin_wayland_wrapper-wrapped", "kwin_wayland_wrapper"), + ("foo-wrapped", "foo-wrapped"), + (".hidden", ".hidden"), + (".-wrapped", ".-wrapped"), + ] { + assert_eq!(undecorate(raw), want, "undecorate({raw:?})"); + } + } + + /// The whole bug. Every compositor the session probe matches on is wrapped by nixpkgs, so the + /// kernel reports a truncated, decorated `comm` that can never equal the name being compared — + /// which is why `detect_active_session` answered `ActiveKind::None` on a *running* KDE desktop + /// and every connect died "no usable compositor". + #[test] + fn a_nixpkgs_wrapped_compositor_resolves_to_its_real_name() { + for (tag, comm, exe, want) in [ + ( + "kwin", + ".kwin_wayland-w", + ".kwin_wayland-wrapped", + "kwin_wayland", + ), + ( + "gamescope", + ".gamescope-wrap", + ".gamescope-wrapped", + "gamescope", + ), + ( + "gnome", + ".gnome-shell-wr", + ".gnome-shell-wrapped", + "gnome-shell", + ), + ("hypr", ".Hyprland-wrapp", ".Hyprland-wrapped", "Hyprland"), + ] { + let p = FakePid::new(tag, comm, Some(exe)); + assert_eq!( + match_name(p.path()).as_deref(), + Some(want), + "a nixpkgs-wrapped {want} must resolve to the name the session probe matches" + ); + } + } + + /// KWin's own `kwin_wayland_wrapper` is a real binary that runs *alongside* `kwin_wayland`, and + /// its wrapped `comm` (`.kwin_wayland_w`) differs from the compositor's by a single byte. It + /// must NOT resolve to `kwin_wayland`: the probe would then match the parent process and carry + /// its PID as the compositor identity, which drives restart detection. + #[test] + fn kwins_own_wrapper_binary_does_not_masquerade_as_the_compositor() { + let p = FakePid::new( + "kwrap", + ".kwin_wayland_w", + Some(".kwin_wayland_wrapper-wrapped"), + ); + assert_eq!( + match_name(p.path()).as_deref(), + Some("kwin_wayland_wrapper") + ); + } + + /// The other half of the 15-byte limit, with no nix involved: a long name is truncated too, and + /// has to be recovered from `exe` rather than matched short. + #[test] + fn a_long_name_is_recovered_untruncated() { + let p = FakePid::new( + "long", + "a-very-long-com", + Some("a-very-long-compositor-name"), + ); + assert_eq!( + match_name(p.path()).as_deref(), + Some("a-very-long-compositor-name") + ); + } + + /// The fast path answers without consulting `exe` at all — which is what keeps this probe at one + /// read per process on every ordinary distro, and what lets it answer for a process whose `exe` + /// is unreadable in the first place. + #[test] + fn an_ordinary_short_name_never_needs_the_exe_link() { + let p = FakePid::new("plain", "kwin_wayland", None); + assert_eq!(match_name(p.path()).as_deref(), Some("kwin_wayland")); + } + + /// A decorated-or-truncated name whose `exe` cannot be read (a kernel thread, or a process + /// exiting under the scan) degrades to the truncated `comm` instead of failing the whole entry. + #[test] + fn an_unreadable_exe_falls_back_to_comm() { + let p = FakePid::new("noexe", ".kwin_wayland-w", None); + assert_eq!(match_name(p.path()).as_deref(), Some(".kwin_wayland-w")); + } + + /// A pid directory that does not exist yields `None`, not a bogus name — the scans `continue`. + #[test] + fn a_vanished_process_yields_none() { + assert_eq!(match_name(Path::new("/proc/0")), None); + } + + /// The one thing a fixture cannot establish: that reading `/proc//exe` is actually + /// *permitted* for a process of our own uid, which the whole resolver depends on. Checked + /// against the only such process guaranteed to be running — this one. + #[test] + fn our_own_exe_link_is_readable() { + let me = Path::new("/proc/self"); + let exe = std::fs::read_link(me.join("exe")) + .expect("/proc/self/exe must be readable for our own uid"); + let name = exe.file_name().and_then(|n| n.to_str()).expect("exe name"); + let got = match_name(me).expect("our own name"); + // Whichever rung answered, it must agree with the real binary: the fast path returns the + // (short, undecorated) comm, which is a prefix of it; the exe path returns it outright. + assert!( + name.starts_with(got.as_str()) || got == name, + "resolved {got:?} disagrees with our real binary {name:?}" + ); + } +} + /// The same two cases through `cmd /c`, so the budget logic is covered on the platform whose /// process model differs most (job objects, no `SIGKILL`). `ping -n` is the standard Windows /// no-extra-tooling sleep. diff --git a/crates/pf-vdisplay/src/vdisplay/session.rs b/crates/pf-vdisplay/src/vdisplay/session.rs index d95e6cc9..e99b6f88 100644 --- a/crates/pf-vdisplay/src/vdisplay/session.rs +++ b/crates/pf-vdisplay/src/vdisplay/session.rs @@ -311,8 +311,11 @@ pub fn detect_active_session() -> ActiveSession { 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 - // `pkill -x` discipline (exact, ≤15 chars so untruncated). + // a real desktop (kwin/gnome/sway) win over a leftover gamescope child. Names are matched + // exactly, `pkill -x` style — but resolved through [`crate::proc::match_name`], NOT a raw + // `comm` read: on NixOS every one of these binaries is a nixpkgs wrapper whose real ELF is + // `.-wrapped`, so a raw `comm` says `.kwin_wayland-w` and this whole probe answered + // `None` on a running KDE desktop. let mut kind = ActiveKind::None; let mut best = 0u8; // The winning compositor's PID — kept so a same-kind compositor RESTART (a new PID) bumps the @@ -332,10 +335,10 @@ pub fn detect_active_session() -> ActiveSession { if md.uid() != uid { continue; } - let Ok(comm) = std::fs::read_to_string(pid_path.join("comm")) else { + let Some(comm) = crate::proc::match_name(&pid_path) else { continue; }; - let (k, prio) = match comm.trim() { + let (k, prio) = match comm.as_str() { "gamescope" | "gamescope-wl" => (ActiveKind::Gaming, 1), "kwin_wayland" => (ActiveKind::DesktopKde, 4), "gnome-shell" => (ActiveKind::DesktopGnome, 4),