diff --git a/Cargo.lock b/Cargo.lock index 0388e749..f90f5f8b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3599,6 +3599,7 @@ dependencies = [ "crossterm 0.29.0", "flate2", "keyring-core", + "libc", "rocm-core", "rocm-dash-daemon", "rocm-dash-tui", diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 3984ca5a..fc33aefb 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -38,6 +38,9 @@ tracing-appender = "0.2" tracing-subscriber = { version = "0.3", features = ["env-filter"] } ureq = { version = "2.12", features = ["native-certs"] } +[target.'cfg(unix)'.dependencies] +libc.workspace = true + [target.'cfg(target_os = "windows")'.dependencies] windows-native-keyring-store = "1.1" diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index b0a3fd13..6dd6b9f1 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -878,7 +878,57 @@ impl PermissionsModeArg { } } +/// Restore the default SIGPIPE disposition on Unix. +/// +/// Rust installs `SIG_IGN` for SIGPIPE at startup, so a downstream reader +/// closing the pipe early (e.g. `rocm fix … --dry-run | head`) turns the next +/// `println!` into a panic (`failed printing to stdout: Broken pipe`, exit 101) +/// instead of the conventional quiet exit. Resetting to `SIG_DFL` makes the +/// process terminate on SIGPIPE like every other Unix CLI. Socket writes are +/// unaffected: std uses `MSG_NOSIGNAL`, so the serve/daemon paths still see a +/// normal `BrokenPipe` error rather than a signal. +#[cfg(unix)] +#[allow(unsafe_code)] // libc FFI +fn reset_sigpipe() { + // SAFETY: installing the default handler for a signal is always safe. + unsafe { + libc::signal(libc::SIGPIPE, libc::SIG_DFL); + } +} + +#[cfg(not(unix))] +fn reset_sigpipe() {} + +/// Run `f` with SIGPIPE temporarily ignored, restoring the previous +/// disposition afterwards. +/// +/// [`reset_sigpipe`] sets `SIG_DFL` process-wide so stdout/stderr pipe writes +/// terminate the process conventionally. But a write to a spawned child's stdin +/// whose reader has already exited would then kill `rocm` with SIGPIPE before +/// the caller can turn the resulting `EPIPE` into a diagnostic. Wrap such writes +/// in this helper so the failure surfaces as an `io::Error` (`BrokenPipe`) +/// instead of a signal. +#[cfg(unix)] +#[allow(unsafe_code)] // libc FFI +fn with_sigpipe_ignored(f: impl FnOnce() -> T) -> T { + // SAFETY: reading/installing a signal disposition is always safe. + let previous = unsafe { libc::signal(libc::SIGPIPE, libc::SIG_IGN) }; + let result = f(); + // SAFETY: restoring the disposition captured above is always safe. + unsafe { + libc::signal(libc::SIGPIPE, previous); + } + result +} + +#[cfg(not(unix))] +fn with_sigpipe_ignored(f: impl FnOnce() -> T) -> T { + f() +} + fn main() -> Result<()> { + reset_sigpipe(); + // Held for the whole process lifetime: dropping it flushes and stops the // non-blocking file writer, so an early drop would silently truncate the // log. A failed/missing `AppPaths::discover()` degrades to no logging @@ -15209,8 +15259,18 @@ where .stdin .take() .context("engine stdio child did not expose stdin")?; - serde_json::to_writer(&mut stdin, &envelope).context("failed to write engine request")?; - stdin.write_all(b"\n")?; + // Ignore SIGPIPE for the duration of the write: with SIG_DFL in effect + // process-wide, an engine that exits before reading its stdin would + // otherwise kill `rocm` with SIGPIPE before the exit-status diagnostics + // below can run. Under SIG_IGN the write returns EPIPE instead, which we + // surface as the normal "failed to write engine request" error and then + // let `wait()` report why the engine exited. + with_sigpipe_ignored(|| { + serde_json::to_writer(&mut stdin, &envelope) + .context("failed to write engine request")?; + stdin.write_all(b"\n")?; + Ok::<(), anyhow::Error>(()) + })?; } let stderr_handle = child.stderr.take().map(|stderr| { @@ -16284,6 +16344,89 @@ mod tests { Cli::command().debug_assert(); } + /// Regression test for the engine child-stdin write under the process-wide + /// `SIG_DFL` that `main` installs via [`reset_sigpipe`]. If an engine child + /// exits before reading its stdin, the parent's write to that pipe must + /// surface as a `BrokenPipe` error — handled by the caller's diagnostics — + /// rather than killing `rocm` with SIGPIPE before those diagnostics run. + /// + /// The bug only reproduces under `SIG_DFL`; the test harness leaves SIGPIPE + /// at Rust's default `SIG_IGN`, and flipping it process-wide would race + /// sibling tests. So this re-execs itself in fresh processes that install + /// `SIG_DFL` first, and checks BOTH directions: + /// - unguarded write to a dead child's stdin is fatal (killed by SIGPIPE) — + /// this is the regression the plain `SIG_DFL` reset introduced; + /// - the same write wrapped in [`with_sigpipe_ignored`] survives and returns + /// an error instead — this is the fix. + /// + /// Each child reaps its short-lived grandchild before writing, so the read + /// end is closed deterministically and the outcome does not race. + #[cfg(unix)] + #[test] + fn engine_stdin_write_under_sig_dfl_is_guarded_not_fatal() { + use std::os::unix::process::ExitStatusExt as _; + + // Grandchild helper: reproduce main()'s SIG_DFL, spawn a process that + // exits immediately, reap it, then write to its now-closed stdin. The + // "guarded" arm wraps the write; the "unguarded" arm does not. + if let Some(mode) = std::env::var_os("ROCM_TEST_SIGPIPE_ARM") { + reset_sigpipe(); + let mut grandchild = ProcessCommand::new("true") + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn a process that exits immediately"); + let mut stdin = grandchild.stdin.take().expect("grandchild stdin"); + grandchild.wait().expect("reap grandchild"); + // Heap-allocated so the buffer doesn't trip clippy::large-stack-arrays; + // it must exceed the pipe buffer so the write actually reaches the + // closed read end rather than being swallowed by kernel buffering. + let payload = vec![b'x'; 64 * 1024]; + if mode == "guarded" { + let result = with_sigpipe_ignored(|| stdin.write_all(&payload)); + assert!( + result.is_err(), + "guarded write to a dead pipe should return BrokenPipe, not succeed" + ); + } else { + // Unguarded: under SIG_DFL this write delivers SIGPIPE and never + // returns. If the process somehow survives, exit 0 so the parent's + // "should have been signalled" assertion fails loudly. + let _ = stdin.write_all(&payload); + } + return; + } + + let exe = std::env::current_exe().expect("current test executable"); + let run_arm = |mode: &str| -> ExitStatus { + ProcessCommand::new(&exe) + .args([ + "tests::engine_stdin_write_under_sig_dfl_is_guarded_not_fatal", + "--exact", + "--nocapture", + ]) + .env("ROCM_TEST_SIGPIPE_ARM", mode) + .status() + .expect("re-exec the test arm in a child process") + }; + + let unguarded = run_arm("unguarded"); + assert_eq!( + unguarded.signal(), + Some(libc::SIGPIPE), + "sanity check: an unguarded stdin write under SIG_DFL must be killed by \ + SIGPIPE (got {unguarded:?}); if not, the test no longer proves the guard matters" + ); + + let guarded = run_arm("guarded"); + assert!( + guarded.success(), + "guarded child-stdin write was fatal under SIG_DFL ({guarded:?}) — \ + SIGPIPE reached the process instead of surfacing as an error" + ); + } + #[test] fn bench_load_rejects_zero_and_unbounded_numeric_arguments() { for args in [ diff --git a/apps/rocm/tests/broken_pipe.rs b/apps/rocm/tests/broken_pipe.rs new file mode 100644 index 00000000..578085ab --- /dev/null +++ b/apps/rocm/tests/broken_pipe.rs @@ -0,0 +1,56 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Regression test: a downstream reader closing the pipe early must not make +//! `rocm` panic. Rust installs `SIG_IGN` for SIGPIPE at startup, so without the +//! `reset_sigpipe()` in `main` the next `println!` after the pipe closes panics +//! with `failed printing to stdout: Broken pipe` and exit code 101 (found via +//! `rocm fix fix-2-unset-override --dry-run | head`). With SIGPIPE reset to +//! `SIG_DFL` the process instead terminates on the signal (exit 128+13 = 141), +//! the conventional behaviour for a Unix CLI, and never prints a panic. + +#![cfg(unix)] + +use std::io::Read; +use std::process::{Command, Stdio}; + +/// Spawn a command whose output far exceeds the OS pipe buffer (`completions +/// bash` is ~120 KB, vs a 64 KB pipe), read a single byte, then drop the read +/// end so the child's next write hits the closed pipe while it is still +/// producing output. A small-output command (e.g. `fix`, ~1.5 KB) would fit +/// entirely in the pipe buffer and let the child finish before the close, +/// never exercising the broken-pipe path. The SIGPIPE reset lives in `main`, +/// so this covers every subcommand, not just the one that first surfaced it. +#[test] +fn early_pipe_close_does_not_panic() { + let mut child = Command::new(env!("CARGO_BIN_EXE_rocm")) + .args(["completions", "bash"]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()) + .spawn() + .expect("spawn rocm completions bash"); + + // Read a single byte, then drop stdout to close the read end of the pipe. + let mut stdout = child.stdout.take().expect("capture stdout"); + let mut one = [0u8; 1]; + let _ = stdout.read(&mut one); + drop(stdout); + + let mut stderr = child.stderr.take().expect("capture stderr"); + let mut err = String::new(); + let _ = stderr.read_to_string(&mut err); + + let status = child.wait().expect("wait for rocm completions bash"); + + assert!( + !err.contains("panicked") && !err.contains("Broken pipe"), + "rocm panicked on a closed stdout pipe instead of exiting on SIGPIPE; stderr:\n{err}" + ); + assert_ne!( + status.code(), + Some(101), + "rocm exited 101 (panic) on a closed stdout pipe; expected clean SIGPIPE termination" + ); +}