Skip to content

yes, timeout: adding pipeline signal handlers #10166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ChrisDryden wants to merge 1 commit into from
Closed

yes, timeout: adding pipeline signal handlers #10166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions src/uu/timeout/src/timeout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ use uucore::parser::parse_time;
use uucore::process::ChildExt;
use uucore::translate;

#[cfg(unix)]
use uucore::signals::enable_pipe_errors;

use uucore::{
format_usage, show_error,
signals::{signal_by_name_or_value, signal_name_by_value},
Expand Down Expand Up @@ -105,8 +102,16 @@ impl Config {
}
}

#[cfg(unix)]
uucore::init_sigpipe_capture!();

#[uucore::main]
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
#[cfg(unix)]
if !uucore::signals::sigpipe_was_ignored() {
uucore::signals::enable_pipe_errors()?;
}

let matches =
uucore::clap_localization::handle_clap_result_with_exit_code(uu_app(), args, 125)?;

Expand Down Expand Up @@ -320,26 +325,28 @@ fn timeout(
if !foreground {
let _ = setpgid(Pid::from_raw(0), Pid::from_raw(0));
}
#[cfg(unix)]
enable_pipe_errors()?;

let process = &mut process::Command::new(&cmd[0])
let mut cmd_builder = process::Command::new(&cmd[0]);
cmd_builder
.args(&cmd[1..])
.stdin(Stdio::inherit())
.stdout(Stdio::inherit())
.stderr(Stdio::inherit())
.spawn()
.map_err(|err| {
let status_code = match err.kind() {
ErrorKind::NotFound => ExitStatus::CommandNotFound.into(),
ErrorKind::PermissionDenied => ExitStatus::CannotInvoke.into(),
_ => ExitStatus::CannotInvoke.into(),
};
USimpleError::new(
status_code,
translate!("timeout-error-failed-to-execute-process", "error" => err),
)
})?;
.stderr(Stdio::inherit());

#[cfg(unix)]
uucore::signals::preserve_sigpipe_for_child(&mut cmd_builder);

let process = &mut cmd_builder.spawn().map_err(|err| {
let status_code = match err.kind() {
ErrorKind::NotFound => ExitStatus::CommandNotFound.into(),
ErrorKind::PermissionDenied => ExitStatus::CannotInvoke.into(),
_ => ExitStatus::CannotInvoke.into(),
};
USimpleError::new(
status_code,
translate!("timeout-error-failed-to-execute-process", "error" => err),
)
})?;
unblock_sigchld();
catch_sigterm();
// Wait for the child process for the specified time period.
Expand Down
23 changes: 18 additions & 5 deletions src/uu/yes/src/yes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,22 @@ use std::ffi::OsString;
use std::io::{self, Write};
use uucore::error::{UResult, USimpleError};
use uucore::format_usage;
#[cfg(unix)]
use uucore::signals::enable_pipe_errors;
use uucore::translate;

// it's possible that using a smaller or larger buffer might provide better performance on some
// systems, but honestly this is good enough
const BUF_SIZE: usize = 16 * 1024;

#[cfg(unix)]
uucore::init_sigpipe_capture!();

#[uucore::main]
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
#[cfg(unix)]
if !uucore::signals::sigpipe_was_ignored() {
let _ = uucore::signals::enable_pipe_errors();
}

let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?;

let mut buffer = Vec::with_capacity(BUF_SIZE);
Expand All @@ -29,7 +35,16 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {

match exec(&buffer) {
Ok(()) => Ok(()),
Err(err) if err.kind() == io::ErrorKind::BrokenPipe => Ok(()),
Err(err) if err.kind() == io::ErrorKind::BrokenPipe => {
#[cfg(unix)]
if uucore::signals::sigpipe_was_ignored() {
uucore::show_error!(
"{}",
translate!("yes-error-standard-output", "error" => err)
Copy link
Collaborator

@Ecordonnier Ecordonnier Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we should use "err.kind()" instead of "err" in order to match exactly GNU yes error string and remove the "os error 32":

asteba@asteba-MS-7C75:~/dev/coreutils$ trap '' PIPE
asteba@asteba-MS-7C75:~/dev/coreutils$ ./target/debug/coreutils yes | head -1
y
yes: standard output: Broken pipe (os error 32)
asteba@asteba-MS-7C75:~/dev/coreutils$ /usr/bin/gnuyes | head -1
y
gnuyes: standard output: Broken pipe

);
}
Ok(())
}
Err(err) => Err(USimpleError::new(
1,
translate!("yes-error-standard-output", "error" => err),
Expand Down Expand Up @@ -113,8 +128,6 @@ fn prepare_buffer(buf: &mut Vec<u8>) {
pub fn exec(bytes: &[u8]) -> io::Result<()> {
let stdout = io::stdout();
let mut stdout = stdout.lock();
#[cfg(unix)]
enable_pipe_errors()?;

loop {
stdout.write_all(bytes)?;
Expand Down
19 changes: 19 additions & 0 deletions src/uucore/src/lib/features/signals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,25 @@ pub const fn sigpipe_was_ignored() -> bool {
false
}

/// Configures a Command to preserve SIGPIPE disposition for child processes.
/// When the parent had SIGPIPE ignored, this ensures the child also has it ignored.
#[cfg(unix)]
Copy link
Collaborator

@Ecordonnier Ecordonnier Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining that Rust's stdlib resets SIGPIPE to SIG_DFL when spawning child processes (Rust source: https://github.com/rust-lang/rust/blob/fe98ddcfcfb6f185dbf4adeaf439d8a756da0273/library/std/src/sys/process/unix/unix.rs#L742 ), unless the -Zon-broken-pipe flag is used. This breaks Unix signal inheritance when the parent has SIGPIPE ignored, which is why preserve_sigpipe_for_child() uses pre_exec() to restore the ignored state before exec.

It is critical context that the rust stdlib restores SIGPIPE to SIG_DFL when calling Command functions. Without this context, I'm concerned that future readers would not understand the role of preserve_sigpipe_for_child().

pub fn preserve_sigpipe_for_child(cmd: &mut std::process::Command) {
use std::os::unix::process::CommandExt;
if sigpipe_was_ignored() {
unsafe {
cmd.pre_exec(|| {
nix::sys::signal::signal(
nix::sys::signal::Signal::SIGPIPE,
nix::sys::signal::SigHandler::SigIgn,
)
.map_err(std::io::Error::other)?;
Ok(())
});
}
}
}

#[cfg(target_os = "linux")]
pub fn ensure_stdout_not_broken() -> std::io::Result<bool> {
use nix::{
Expand Down
18 changes: 18 additions & 0 deletions tests/by-util/test_timeout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ use rstest::rstest;

use uucore::display::Quotable;
use uutests::new_ucmd;
#[cfg(all(unix, feature = "yes"))]
use uutests::util::TestScenario;
#[cfg(all(unix, feature = "yes"))]
use uutests::util_name;

#[test]
fn test_invalid_arg() {
Expand Down Expand Up @@ -235,3 +239,17 @@ fn test_command_cannot_invoke() {
// Try to execute a directory (should give permission denied or similar)
new_ucmd!().args(&["1", "/"]).fails_with_code(126);
}

/// Test for https://github.com/uutils/coreutils/issues/7252
#[test]
#[cfg(unix)]
#[cfg(feature = "yes")]
fn test_sigpipe_ignored_preserves_for_child() {
Copy link
Collaborator

@Ecordonnier Ecordonnier Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn test_sigpipe_ignored_preserves_for_child() {
fn test_sigpipe_ignored_preserved_for_child() {

let ts = TestScenario::new(util_name!());
let bin = ts.bin_path.clone().into_os_string();
let result = ts
.cmd_shell("trap '' PIPE; \"$BIN\" timeout 10 \"$BIN\" yes 2>err |:; cat err")
.env("BIN", &bin)
.succeeds();
assert!(result.stdout_str().contains("Broken pipe"));
}
17 changes: 17 additions & 0 deletions tests/by-util/test_yes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ use std::process::ExitStatus;
use std::os::unix::process::ExitStatusExt;

use uutests::new_ucmd;
#[cfg(unix)]
use uutests::util::TestScenario;
#[cfg(unix)]
use uutests::util_name;

#[cfg(unix)]
fn check_termination(result: ExitStatus) {
Expand Down Expand Up @@ -111,3 +115,16 @@ fn test_non_utf8() {
&b"\xbf\xff\xee bar\n".repeat(5000),
);
}

/// Test for https://github.com/uutils/coreutils/issues/7252
#[test]
#[cfg(unix)]
fn test_sigpipe_ignored_prints_error() {
let ts = TestScenario::new(util_name!());
let bin = ts.bin_path.clone().into_os_string();
let result = ts
.cmd_shell("trap '' PIPE; \"$BIN\" yes 2>err |:; cat err")
.env("BIN", &bin)
.succeeds();
assert!(result.stdout_str().contains("Broken pipe"));
}
Loading