From dd9bbaf1c5181f6122ca89eb7caee9f737ed28d1 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 08:22:16 +0200 Subject: [PATCH] fix(pf-vdisplay): a helper that outran the pipe buffer had its output thrown away as a timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `output_within` read stdout/stderr only after the child exited, and its doc justified that with "these helpers emit at most a few hundred KiB, well under any real pipe pressure". A pipe holds 64 KiB. Anything past that blocks the helper in `write()`, so it never exits, the budget kills it, and a successful query is reported to the caller as `TimedOut` with its answer discarded. The busiest caller is the one that trips it: `pw-dump` on a populated PipeWire graph clears 64 KiB routinely and is polled from the 45 s gamescope loops. Confirmed empirically — a child writing 1 MiB into an undrained pipe never exits. Both pipes are now drained on their own threads, concurrently with the wait. That makes the joins load-bearing, which exposed the second half: the Unix `tree::Guard` was an empty stub whose doc claimed `Child::kill` "already ends the only process there is". It never did for this crate's Linux helpers — `pkexec`, `systemd-run`, `systemctl --user` and the `sh -c` wrappers all fork — and a surviving grandchild holds the pipes' write ends, so a reader would wait for an EOF that never arrives. The child is now the leader of its own process group and the guard `killpg`s it, which is the Unix shape of the Job object the Windows half already used. Also gates `pf-frame`, `pf-gpu` and `pf-encode` to Windows: every use site of all three is `cfg(windows)`, and between them they dragged FFmpeg, ash and openh264 into the Linux build for nothing (sweep item 13.19). --- crates/pf-vdisplay/Cargo.toml | 19 +++- crates/pf-vdisplay/src/vdisplay/policy.rs | 4 +- crates/pf-vdisplay/src/vdisplay/proc.rs | 127 ++++++++++++++++++---- 3 files changed, 124 insertions(+), 26 deletions(-) diff --git a/crates/pf-vdisplay/Cargo.toml b/crates/pf-vdisplay/Cargo.toml index 4bb903b7..ad571f6f 100644 --- a/crates/pf-vdisplay/Cargo.toml +++ b/crates/pf-vdisplay/Cargo.toml @@ -16,13 +16,9 @@ publish = false [dependencies] punktfunk-core = { path = "../punktfunk-core", features = ["quic"] } -pf-frame = { path = "../pf-frame" } -pf-gpu = { path = "../pf-gpu" } pf-host-config = { path = "../pf-host-config" } pf-paths = { path = "../pf-paths" } pf-win-display = { path = "../pf-win-display" } -# The Windows admission gate consults NVENC's session budget (can_open_another_session). -pf-encode = { path = "../pf-encode" } anyhow = "1" tracing = "0.1" # The platform-neutral policy/identity/custom-preset state is serde-serialized (persisted + the mgmt @@ -41,8 +37,12 @@ hex = "0.4" # the shipped host's dependency closure through this crate is unchanged. tracing-subscriber = { version = "0.3", features = ["env-filter"] } -[target.'cfg(target_os = "linux")'.dependencies] +# `proc`'s process-group tree guard is Unix-wide, not Linux-only: the module is compiled on every +# platform and its tests run on whatever the developer is sitting at (macOS, here). +[target.'cfg(unix)'.dependencies] libc = "0.2" + +[target.'cfg(target_os = "linux")'.dependencies] # The Mutter backend drives D-Bus RemoteDesktop + ScreenCast.RecordVirtual via ashpd on a tokio # runtime; the gamescope restore worker + portal handshakes use tokio too. ashpd = { version = "0.13", features = ["screencast", "remote_desktop"] } @@ -61,6 +61,15 @@ bitflags = "2" x11rb = { version = "0.13", default-features = false } [target.'cfg(target_os = "windows")'.dependencies] +# Windows-only, all three, and gated here rather than unconditionally so the LINUX build does not +# drag their closures in for nothing: `pf-frame` for the DXGI capture identity + the CTA-861.3 HDR +# luminance fields, `pf-gpu` for the render-adapter LUID, and `pf-encode` for the admission gate's +# NVENC session budget (`can_open_another_session`, admission.rs, itself `#[cfg(windows)]`). Every +# use site of all three is Windows-gated — verified by grep — and between them they pull FFmpeg, +# ash and openh264, none of which a Linux host reaches through this crate. +pf-frame = { path = "../pf-frame" } +pf-gpu = { path = "../pf-gpu" } +pf-encode = { path = "../pf-encode" } # The host<->driver wire contract for the pf-vdisplay IddCx backend (control IOCTLs + Pod structs). pf-driver-proto = { path = "../pf-driver-proto" } bytemuck = { version = "1.19", features = ["derive"] } diff --git a/crates/pf-vdisplay/src/vdisplay/policy.rs b/crates/pf-vdisplay/src/vdisplay/policy.rs index 51918610..ee64b4ea 100644 --- a/crates/pf-vdisplay/src/vdisplay/policy.rs +++ b/crates/pf-vdisplay/src/vdisplay/policy.rs @@ -429,7 +429,7 @@ pub fn preset_fields(preset: Preset) -> Option { } /// The persisted policy store: the loaded file value (or `None` when no file exists) behind its -/// JSON path. Mirrors [`pf_gpu::GpuPrefStore`] — private dir, temp-write + atomic rename, +/// JSON path. Mirrors `pf_gpu::GpuPrefStore` — private dir, temp-write + atomic rename, /// in-memory rollback if the disk write fails. pub struct DisplayPolicyStore { path: PathBuf, @@ -513,7 +513,7 @@ impl DisplayPolicyStore { } /// The process-wide display-policy store (config-dir file), loaded once on first access — the same -/// global-accessor shape as [`pf_gpu::prefs`], because display setup happens deep in the +/// global-accessor shape as `pf_gpu::prefs`, because display setup happens deep in the /// capture/vdisplay path where no app state is threaded. pub fn prefs() -> &'static DisplayPolicyStore { static STORE: OnceLock = OnceLock::new(); diff --git a/crates/pf-vdisplay/src/vdisplay/proc.rs b/crates/pf-vdisplay/src/vdisplay/proc.rs index a2971875..2d5b916f 100644 --- a/crates/pf-vdisplay/src/vdisplay/proc.rs +++ b/crates/pf-vdisplay/src/vdisplay/proc.rs @@ -27,6 +27,7 @@ const POLL: Duration = Duration::from_millis(20); /// Stdout/stderr are left as the caller configured them (inherited by default), so this is for /// commands run for their exit status alone — see [`output_within`] when the output is read. pub(crate) fn status_within(cmd: &mut Command, budget: Duration) -> Result { + tree::prepare(cmd); let mut child = cmd.spawn()?; let tree = tree::Guard::attach(&child); let deadline = Instant::now() + budget; @@ -51,35 +52,65 @@ pub(crate) fn status_within(cmd: &mut Command, budget: Duration) -> Result Result { + tree::prepare(cmd); let mut child = cmd .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) .spawn()?; let tree = tree::Guard::attach(&child); + // Taken off the `Child` so the reader threads own them outright: `wait_with_output` must not + // also be reading these, and `try_wait` below needs `&mut child` while they run. + let (stdout, stderr) = (child.stdout.take(), child.stderr.take()); + let (out_rx, err_rx) = (drain(stdout), drain(stderr)); + let deadline = Instant::now() + budget; - loop { + let status = loop { match child.try_wait()? { - Some(_) => { - // Exited: `wait_with_output` now only drains already-buffered pipes — but only if - // nothing else still holds their WRITE end. A grandchild that outlived the helper - // does, and `wait_with_output` reads to an EOF that would then never arrive, which - // is the one way this "bounded" helper could still hang forever. End the tree first. + Some(status) => { + // The helper is gone, but a grandchild it left behind still holds the pipes' WRITE + // ends, so the readers below would wait for an EOF that never arrives. Ending the + // tree closes them — this is what makes the joins bounded. tree.terminate(); - return child.wait_with_output(); + break status; } None if Instant::now() >= deadline => { tree.terminate(); let _ = child.kill(); - let _ = child.wait(); + let _ = child.wait(); // reap it — never leave a zombie behind return Err(timed_out(cmd, budget)); } None => std::thread::sleep(POLL), } - } + }; + // A panicking reader thread cannot lose the call, only its half of the output. + let stdout = out_rx.join().unwrap_or_default(); + let stderr = err_rx.join().unwrap_or_default(); + Ok(Output { + status, + stdout, + stderr, + }) +} + +/// Read one of a child's pipes to EOF on its own thread, so the child never blocks in `write()` +/// waiting for us to catch up. Returns whatever was read; a read error yields the partial buffer, +/// because the caller's failure signal is the budget, not a short pipe. +fn drain(pipe: Option) -> std::thread::JoinHandle> { + std::thread::spawn(move || { + let mut buf = Vec::new(); + if let Some(mut r) = pipe { + let _ = r.read_to_end(&mut buf); + } + buf + }) } fn timed_out(cmd: &Command, budget: Duration) -> Error { @@ -215,6 +246,10 @@ mod tree { JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, }; + /// Nothing to arrange before the spawn: job membership is assigned to the live process, so + /// [`Guard::attach`] does all of it. The Unix twin has to act here instead. + pub(super) fn prepare(_cmd: &mut std::process::Command) {} + /// Owns a Job object holding the spawned helper and everything it spawns. `None` when the job /// could not be set up (see the module doc: degrade, don't fail). pub(super) struct Guard(Option); @@ -294,18 +329,54 @@ mod tree { } } -/// The Unix half: `Child::kill` already ends the only process there is (see the Windows module doc -/// for why that is not true there). Kept as a real type rather than `cfg`ing the call sites, so the -/// two platforms read as one flow. +/// The Unix half — a **process group**, which is what Unix offers in place of a Job object. +/// +/// This used to be an empty stub whose doc said `Child::kill` "already ends the only process there +/// is". That was never true of this crate's Linux helpers, which are the ones the module header is +/// about: `pkexec`, `systemd-run`, `systemctl --user` and the `sh -c` wrappers all fork, so the +/// process that hangs is routinely a grandchild `Child::kill` cannot reach — and with the reader +/// threads in [`output_within`] blocking until every write end of the pipe closes, a surviving +/// grandchild is exactly what would keep a "bounded" call waiting forever. +/// +/// [`prepare`] puts the child in a new process group (it becomes the leader, so the group id is its +/// pid) and [`Guard::terminate`] `killpg`s that group, reaching every descendant that has not +/// deliberately left it. `process_group` changes only the group — not the session — so the helper +/// keeps its controlling terminal and login session, which `pkexec`'s polkit session lookup needs. +/// +/// Best-effort in the same way as the Windows half: a failed `killpg` is ignored, and the +/// single-process `Child::kill` on the timeout path still runs. #[cfg(not(windows))] mod tree { - pub(super) struct Guard; + use std::os::unix::process::CommandExt; + + /// The child's process-group id, captured while the child is still ours to reap. + pub(super) struct Guard(Option); + + /// Make the child the leader of its own process group, so its descendants are reachable as one. + pub(super) fn prepare(cmd: &mut std::process::Command) { + cmd.process_group(0); + } impl Guard { - pub(super) fn attach(_child: &std::process::Child) -> Self { - Self + pub(super) fn attach(child: &std::process::Child) -> Self { + // `prepare` asked for `process_group(0)`, so the group id IS the child's pid. + Self(i32::try_from(child.id()).ok()) + } + + /// End every process still in the group. A no-op once they have all exited, so this is safe + /// to call on the success path as well as the timeout one. + pub(super) fn terminate(&self) { + let Some(pgid) = self.0 else { return }; + // `killpg` is a signal to a group we created and whose leader is the child we spawned; + // it cannot name a process we did not start. The one theoretical hazard is pid reuse + // between the leader's reap and this call, which needs a brand-new process to land on + // exactly that pid AND be a group leader — Linux hands out pids sequentially to + // `pid_max`, so there is no window to speak of, and the alternative (not killing) is + // the unbounded wait this module exists to prevent. + // SAFETY: a plain signal send by group id. No pointer is passed, nothing is aliased, + // and the result is deliberately ignored — ESRCH just means the group is already gone. + unsafe { libc::killpg(pgid, libc::SIGKILL) }; } - pub(super) fn terminate(&self) {} } } @@ -332,6 +403,24 @@ mod tests { ); } + /// A helper whose output exceeds one pipe buffer must still be captured IN FULL. + /// + /// This is the case that fails against a `wait_with_output`-after-exit implementation: the + /// child blocks in `write()` with the pipe full, never exits, and the budget turns a perfectly + /// successful query into a `TimedOut` with its output thrown away. 1 MiB is ~16× a Linux pipe + /// (64 KiB) and ~64× the smallest macOS one, so it cannot be absorbed by a buffer on either. + #[test] + fn a_child_that_outruns_the_pipe_buffer_is_captured_in_full() { + const BYTES: usize = 1024 * 1024; + let mut cmd = Command::new("sh"); + cmd.arg("-c") + .arg(format!("yes punktfunk | head -c {BYTES}; echo done >&2")); + let out = output_within(&mut cmd, Duration::from_secs(20)).expect("must not time out"); + assert!(out.status.success(), "helper failed: {:?}", out.status); + assert_eq!(out.stdout.len(), BYTES, "stdout was truncated"); + assert_eq!(String::from_utf8_lossy(&out.stderr).trim(), "done"); + } + /// The normal path is unaffected: a quick command still yields its status and its output. #[test] fn a_quick_child_returns_normally() {