From 11708b5c01d9a8dd0d4125a83a5ceab60a9c9874 Mon Sep 17 00:00:00 2001 From: Etienne Cordonnier Date: Tue, 9 Dec 2025 21:23:12 +0100 Subject: [PATCH] env: fix "--ignore-signal=PIPE" - revert https://github.com/uutils/coreutils/pull/9614 because `Command::exec()` resets the default signal handler for SIGPIPE, interfering with the option `--ignore-signal=PIPE` - add regression-test for --ignore-signal=PIPE Signed-off-by: Etienne Cordonnier --- src/uu/env/src/env.rs | 57 +++++++++++++++++++++------- tests/by-util/test_env.rs | 78 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 14 deletions(-) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index c12cc671b7a..aab1ef48296 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -24,18 +24,20 @@ use nix::sys::signal::{ SigHandler::{SigDfl, SigIgn}, SigSet, SigmaskHow, Signal, signal, sigprocmask, }; +#[cfg(unix)] +use nix::unistd::execvp; use std::borrow::Cow; #[cfg(unix)] use std::collections::{BTreeMap, BTreeSet}; use std::env; +#[cfg(unix)] +use std::ffi::CString; use std::ffi::{OsStr, OsString}; use std::io; use std::io::Write as _; use std::io::stderr; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; -#[cfg(unix)] -use std::os::unix::process::CommandExt; use uucore::display::{Quotable, print_all_env_vars}; use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError}; @@ -784,16 +786,40 @@ impl EnvAppData { #[cfg(unix)] { - // Execute the program using exec, which replaces the current process. - let err = std::process::Command::new(&*prog) - .arg0(&*arg0) - .args(args) - .exec(); - - // exec() only returns if there was an error - match err.kind() { - io::ErrorKind::NotFound => Err(self.make_error_no_such_file_or_dir(&prog)), - io::ErrorKind::PermissionDenied => { + // Use execvp() directly to preserve signal handlers set by apply_signal_action(). + // Command::exec() would reset SIGPIPE, interfering with --ignore-signal=PIPE. + + // Convert program name to CString. + let prog_os: &OsStr = prog.as_ref(); + let Ok(prog_cstring) = CString::new(prog_os.as_bytes()) else { + return Err(self.make_error_no_such_file_or_dir(&prog)); + }; + + // Prepare arguments for execvp. + let mut argv = Vec::new(); + + // Convert arg0 to CString. + let arg0_os: &OsStr = arg0.as_ref(); + let Ok(arg0_cstring) = CString::new(arg0_os.as_bytes()) else { + return Err(self.make_error_no_such_file_or_dir(&prog)); + }; + argv.push(arg0_cstring); + + // Convert remaining arguments to CString. + for arg in args { + let arg_os = arg; + let Ok(arg_cstring) = CString::new(arg_os.as_bytes()) else { + return Err(self.make_error_no_such_file_or_dir(&prog)); + }; + argv.push(arg_cstring); + } + + // Execute the program using execvp. this replaces the current + // process. The execvp function takes care of appending a NULL + // argument to the argument list so that we don't have to. + match execvp(&prog_cstring, &argv) { + Err(nix::errno::Errno::ENOENT) => Err(self.make_error_no_such_file_or_dir(&prog)), + Err(nix::errno::Errno::EACCES) => { uucore::show_error!( "{}", translate!( @@ -803,16 +829,19 @@ impl EnvAppData { ); Err(126.into()) } - _ => { + Err(_) => { uucore::show_error!( "{}", translate!( "env-error-unknown", - "error" => err + "error" => "execvp failed" ) ); Err(126.into()) } + Ok(_) => { + unreachable!("execvp should never return on success") + } } } diff --git a/tests/by-util/test_env.rs b/tests/by-util/test_env.rs index 8c488e9f3c1..074d29c29c6 100644 --- a/tests/by-util/test_env.rs +++ b/tests/by-util/test_env.rs @@ -1962,3 +1962,81 @@ fn test_non_utf8_env_vars() { .succeeds() .stdout_contains_bytes(b"NON_UTF8_VAR=hello\x80world"); } + +#[test] +#[cfg(unix)] +fn test_ignore_signal_pipe_broken_pipe_regression() { + // Test that --ignore-signal=PIPE properly ignores SIGPIPE in child processes. + // When SIGPIPE is ignored, processes should handle broken pipes gracefully + // instead of being terminated by the signal. + // + // Regression test for: https://github.com/uutils/coreutils/issues/9617 + + use std::io::{BufRead, BufReader}; + use std::process::{Command, Stdio}; + + let scene = TestScenario::new(util_name!()); + + // Helper function to simulate a broken pipe scenario (like "seq 1000000 | head -n1") + let test_sigpipe_behavior = |use_ignore_signal: bool| -> i32 { + let mut cmd = Command::new(&scene.bin_path); + cmd.arg("env"); + + if use_ignore_signal { + cmd.arg("--ignore-signal=PIPE"); + } + + // Use seq instead of yes - writes bounded output but enough to trigger SIGPIPE + cmd.arg("seq") + .arg("1") + .arg("1000000") + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + + let mut child = cmd.spawn().expect("Failed to spawn env process"); + + // Read exactly one line then close the pipe to trigger SIGPIPE + if let Some(stdout) = child.stdout.take() { + let mut reader = BufReader::new(stdout); + let mut line = String::new(); + let _ = reader.read_line(&mut line); + // Pipe closes when reader is dropped, sending SIGPIPE to writing process + } + + // seq should exit quickly (either from SIGPIPE or after handling EPIPE) + match child.wait() { + Ok(status) => status.code().unwrap_or(141), // 128 + 13 + Err(_) => 141, + } + }; + + // Test without signal ignoring - should be killed by SIGPIPE + let normal_exit_code = test_sigpipe_behavior(false); + println!("Normal 'env seq' exit code: {normal_exit_code}"); + + // Test with --ignore-signal=PIPE - should handle broken pipe gracefully + let ignore_signal_exit_code = test_sigpipe_behavior(true); + println!("With --ignore-signal=PIPE exit code: {ignore_signal_exit_code}"); + + // Verify the --ignore-signal=PIPE flag changes the behavior + assert!( + ignore_signal_exit_code != 141, + "--ignore-signal=PIPE had no effect! Process was still killed by SIGPIPE (exit code 141). Normal: {normal_exit_code}, --ignore-signal: {ignore_signal_exit_code}" + ); + + // Expected behavior: + assert_eq!( + normal_exit_code, 141, + "Without --ignore-signal, process should be killed by SIGPIPE" + ); + assert_ne!( + ignore_signal_exit_code, 141, + "With --ignore-signal=PIPE, process should NOT be killed by SIGPIPE" + ); + + // Process should exit gracefully when SIGPIPE is ignored + assert!( + ignore_signal_exit_code == 0 || ignore_signal_exit_code == 1, + "With --ignore-signal=PIPE, process should exit gracefully (0 or 1), got: {ignore_signal_exit_code}" + ); +}