From 51e0361c5f37bc722cbeaf3faedeeb27b24fa7d9 Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Wed, 10 Dec 2025 17:25:48 +0000 Subject: [PATCH 1/5] Added a bunch of nohup fixes based on the failures in the GNU tests --- Cargo.lock | 1 + src/uu/nohup/Cargo.toml | 1 + src/uu/nohup/locales/en-US.ftl | 2 + src/uu/nohup/src/nohup.rs | 118 +++++++++++++++++++---------- tests/by-util/test_nohup.rs | 132 +++++++++++++++++++++++++++++++-- 5 files changed, 209 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dae4e963cf5..6e074808e37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3603,6 +3603,7 @@ dependencies = [ "clap", "fluent", "libc", + "nix", "thiserror 2.0.17", "uucore", ] diff --git a/src/uu/nohup/Cargo.toml b/src/uu/nohup/Cargo.toml index 50eb258a9f8..6416f3405ec 100644 --- a/src/uu/nohup/Cargo.toml +++ b/src/uu/nohup/Cargo.toml @@ -20,6 +20,7 @@ path = "src/nohup.rs" [dependencies] clap = { workspace = true } libc = { workspace = true } +nix = { workspace = true, features = ["fs"] } uucore = { workspace = true, features = ["fs"] } thiserror = { workspace = true } fluent = { workspace = true } diff --git a/src/uu/nohup/locales/en-US.ftl b/src/uu/nohup/locales/en-US.ftl index abc51841c29..05533a4839d 100644 --- a/src/uu/nohup/locales/en-US.ftl +++ b/src/uu/nohup/locales/en-US.ftl @@ -14,4 +14,6 @@ nohup-error-open-failed-both = failed to open { $first_path }: { $first_err } failed to open { $second_path }: { $second_err } # Status messages +nohup-ignoring-input = ignoring input +nohup-appending-output = appending output to { $path } nohup-ignoring-input-appending-output = ignoring input and appending output to { $path } diff --git a/src/uu/nohup/src/nohup.rs b/src/uu/nohup/src/nohup.rs index 28292ac4154..762f532fbc8 100644 --- a/src/uu/nohup/src/nohup.rs +++ b/src/uu/nohup/src/nohup.rs @@ -7,9 +7,11 @@ use clap::{Arg, ArgAction, Command}; use libc::{SIG_IGN, SIGHUP, dup2, signal}; +use nix::sys::stat::{Mode, umask}; use std::env; use std::fs::{File, OpenOptions}; -use std::io::{Error, ErrorKind, IsTerminal}; +use std::io::{Error, ErrorKind, IsTerminal, Write}; +use std::os::unix::fs::OpenOptionsExt; use std::os::unix::prelude::*; use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; @@ -17,8 +19,8 @@ use std::process; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{UError, UResult, set_exit_code}; +use uucore::format_usage; use uucore::translate; -use uucore::{format_usage, show_error}; static NOHUP_OUT: &str = "nohup.out"; // exit codes that match the GNU implementation @@ -44,21 +46,46 @@ enum NohupError { #[error("{}", translate!("nohup-error-open-failed-both", "first_path" => NOHUP_OUT.quote(), "first_err" => _1, "second_path" => _2.quote(), "second_err" => _3))] OpenFailed2(i32, #[source] Error, String, Error), + + #[error("")] + StderrWriteFailed(i32), } impl UError for NohupError { fn code(&self) -> i32 { match self { - Self::OpenFailed(code, _) | Self::OpenFailed2(code, _, _, _) => *code, + Self::OpenFailed(code, _) + | Self::OpenFailed2(code, _, _, _) + | Self::StderrWriteFailed(code) => *code, _ => 2, } } } +fn failure_code() -> i32 { + match env::var("POSIXLY_CORRECT") { + Ok(_) => POSIX_NOHUP_FAILURE, + Err(_) => EXIT_CANCELED, + } +} + +/// We are unable to use the regular show_error because we need to detect if stderr +/// is unavailable because GNU nohup exits with 125 if it can't write to stderr. +fn write_stderr(msg: &str) -> UResult<()> { + let mut stderr = std::io::stderr(); + if writeln!(stderr, "nohup: {msg}").is_err() || stderr.flush().is_err() { + return Err(NohupError::StderrWriteFailed(failure_code()).into()); + } + Ok(()) +} + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let matches = - uucore::clap_localization::handle_clap_result_with_exit_code(uu_app(), args, 125)?; + let matches = uucore::clap_localization::handle_clap_result_with_exit_code( + uu_app(), + args, + failure_code(), + )?; replace_fds()?; @@ -100,7 +127,10 @@ pub fn uu_app() -> Command { } fn replace_fds() -> UResult<()> { - if std::io::stdin().is_terminal() { + let stdin_is_terminal = std::io::stdin().is_terminal(); + let stdout_is_terminal = std::io::stdout().is_terminal(); + + if stdin_is_terminal { let new_stdin = File::open(Path::new("/dev/null")) .map_err(|e| NohupError::CannotReplace("STDIN", e))?; if unsafe { dup2(new_stdin.as_raw_fd(), 0) } != 0 { @@ -108,13 +138,27 @@ fn replace_fds() -> UResult<()> { } } - if std::io::stdout().is_terminal() { - let new_stdout = find_stdout()?; + if stdout_is_terminal { + let (new_stdout, path) = find_stdout()?; let fd = new_stdout.as_raw_fd(); + // Print the appropriate message based on what we're doing + // Use write_stderr to detect write failures (e.g., /dev/full) + if stdin_is_terminal { + write_stderr(&translate!( + "nohup-ignoring-input-appending-output", + "path" => path.quote() + ))?; + } else { + write_stderr(&translate!("nohup-appending-output", "path" => path.quote()))?; + } + if unsafe { dup2(fd, 1) } != 1 { return Err(NohupError::CannotReplace("STDOUT", Error::last_os_error()).into()); } + } else if stdin_is_terminal { + // Only ignoring input, not redirecting stdout + write_stderr(&translate!("nohup-ignoring-input"))?; } if std::io::stderr().is_terminal() && unsafe { dup2(1, 2) } != 2 { @@ -123,46 +167,40 @@ fn replace_fds() -> UResult<()> { Ok(()) } -fn find_stdout() -> UResult { - let internal_failure_code = match env::var("POSIXLY_CORRECT") { - Ok(_) => POSIX_NOHUP_FAILURE, - Err(_) => EXIT_CANCELED, - }; +/// Open nohup.out file with mode 0o600, temporarily clearing umask. +/// The umask is cleared to ensure the file is created with exactly 0o600 permissions. +fn open_nohup_file(path: &Path) -> std::io::Result { + // Clear umask (set it to 0) and save the old value + let old_umask = umask(Mode::from_bits_truncate(0)); - match OpenOptions::new() + let result = OpenOptions::new() + .write(true) .create(true) .append(true) - .open(Path::new(NOHUP_OUT)) - { - Ok(t) => { - show_error!( - "{}", - translate!("nohup-ignoring-input-appending-output", "path" => NOHUP_OUT.quote()) - ); - Ok(t) - } + .mode(0o600) + .open(path); + + // Restore previous umask + umask(old_umask); + + result +} + +fn find_stdout() -> UResult<(File, String)> { + let exit_code = failure_code(); + + match open_nohup_file(Path::new(NOHUP_OUT)) { + Ok(t) => Ok((t, NOHUP_OUT.to_string())), Err(e1) => { let Ok(home) = env::var("HOME") else { - return Err(NohupError::OpenFailed(internal_failure_code, e1).into()); + return Err(NohupError::OpenFailed(exit_code, e1).into()); }; let mut homeout = PathBuf::from(home); homeout.push(NOHUP_OUT); - let homeout_str = homeout.to_str().unwrap(); - match OpenOptions::new().create(true).append(true).open(&homeout) { - Ok(t) => { - show_error!( - "{}", - translate!("nohup-ignoring-input-appending-output", "path" => homeout_str.quote()) - ); - Ok(t) - } - Err(e2) => Err(NohupError::OpenFailed2( - internal_failure_code, - e1, - homeout_str.to_string(), - e2, - ) - .into()), + let homeout_str = homeout.to_str().unwrap().to_string(); + match open_nohup_file(&homeout) { + Ok(t) => Ok((t, homeout_str)), + Err(e2) => Err(NohupError::OpenFailed2(exit_code, e1, homeout_str, e2).into()), } } } diff --git a/tests/by-util/test_nohup.rs b/tests/by-util/test_nohup.rs index 2349b2dc2a8..ce11a743f38 100644 --- a/tests/by-util/test_nohup.rs +++ b/tests/by-util/test_nohup.rs @@ -3,9 +3,11 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. // spell-checker:ignore winsize Openpty openpty xpixel ypixel ptyprocess +use std::os::unix::fs::PermissionsExt; use std::thread::sleep; use uutests::at_and_ucmd; use uutests::new_ucmd; +use uutests::util::TerminalSimulation; use uutests::util::TestScenario; use uutests::util_name; @@ -13,11 +15,6 @@ use uutests::util_name; // because stdin/stdout is not attached to a TTY. // All that can be tested is the side-effects. -#[test] -fn test_invalid_arg() { - new_ucmd!().arg("--definitely-invalid").fails_with_code(125); -} - #[test] #[cfg(any( target_os = "linux", @@ -238,3 +235,128 @@ fn test_nohup_stderr_to_stdout() { assert!(content.contains("stdout message")); assert!(content.contains("stderr message")); } + +#[test] +fn test_nohup_file_permissions_ignore_umask_always_o600() { + for umask_val in [0o077, 0o000] { + let ts = TestScenario::new(util_name!()); + ts.ucmd() + .terminal_sim_stdio(TerminalSimulation { + stdin: true, + stdout: true, + stderr: true, + size: None, + }) + .umask(umask_val) + .args(&["echo", "test"]) + .succeeds(); + + sleep(std::time::Duration::from_millis(10)); + let mode = std::fs::metadata(ts.fixtures.plus_as_string("nohup.out")) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "with umask {:o}, got mode {:o}", + umask_val, mode + ); + } +} + +#[test] +fn test_nohup_exit_codes() { + // No args: 125 default, 127 with POSIXLY_CORRECT + new_ucmd!().fails_with_code(125); + new_ucmd!().env("POSIXLY_CORRECT", "1").fails_with_code(127); + + // Invalid arg: 125 default, 127 with POSIXLY_CORRECT + new_ucmd!().arg("--invalid").fails_with_code(125); + new_ucmd!() + .env("POSIXLY_CORRECT", "1") + .arg("--invalid") + .fails_with_code(127); +} + +#[test] +fn test_nohup_messages_by_terminal_state() { + let cases = [ + (true, true, "ignoring input and appending output to", ""), + (false, true, "appending output to", "ignoring input"), + (true, false, "ignoring input", "appending output"), + ]; + + for (stdin_tty, stdout_tty, expected, not_expected) in cases { + let ts = TestScenario::new(util_name!()); + let result = ts + .ucmd() + .terminal_sim_stdio(TerminalSimulation { + stdin: stdin_tty, + stdout: stdout_tty, + stderr: true, + size: None, + }) + .args(&["echo", "test"]) + .succeeds(); + + let stderr = String::from_utf8_lossy(result.stderr()); + assert!( + stderr.contains(expected), + "stdin={stdin_tty}, stdout={stdout_tty}: expected '{expected}'" + ); + if !not_expected.is_empty() { + assert!( + !stderr.contains(not_expected), + "stdin={stdin_tty}, stdout={stdout_tty}: unexpected '{not_expected}'" + ); + } + } +} + +#[test] +fn test_nohup_no_message_without_tty() { + new_ucmd!() + .args(&["echo", "test"]) + .succeeds() + .stderr_does_not_contain("ignoring input") + .stderr_does_not_contain("appending output"); +} + +#[test] +#[cfg(target_os = "linux")] +fn test_nohup_stderr_write_failure() { + use std::fs::OpenOptions; + + if !std::path::Path::new("/dev/full").exists() { + return; + } + + for (posixly_correct, expected_code) in [(false, 125), (true, 127)] { + let dev_full = match OpenOptions::new().write(true).open("/dev/full") { + Ok(f) => f, + Err(_) => return, + }; + + let mut cmd = new_ucmd!(); + if posixly_correct { + cmd.env("POSIXLY_CORRECT", "1"); + } + let result = cmd + .terminal_sim_stdio(TerminalSimulation { + stdin: true, + stdout: true, + stderr: false, + size: None, + }) + .set_stderr(dev_full) + .args(&["echo", "test"]) + .fails(); + + assert_eq!( + result.try_exit_status().and_then(|s| s.code()), + Some(expected_code), + "POSIXLY_CORRECT={posixly_correct}" + ); + } +} From 7cd91f9f9259f424fb239cd30ff165d37f24ef3e Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Wed, 10 Dec 2025 17:51:44 +0000 Subject: [PATCH 2/5] Fixing clippy warnings --- src/uu/nohup/src/nohup.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/nohup/src/nohup.rs b/src/uu/nohup/src/nohup.rs index 762f532fbc8..a652fd6b65c 100644 --- a/src/uu/nohup/src/nohup.rs +++ b/src/uu/nohup/src/nohup.rs @@ -174,7 +174,6 @@ fn open_nohup_file(path: &Path) -> std::io::Result { let old_umask = umask(Mode::from_bits_truncate(0)); let result = OpenOptions::new() - .write(true) .create(true) .append(true) .mode(0o600) From 3dd84fa793886e13abb6c8645185216ede0937fa Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Wed, 10 Dec 2025 18:05:04 +0000 Subject: [PATCH 3/5] Fixed clippy issues --- tests/by-util/test_nohup.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/by-util/test_nohup.rs b/tests/by-util/test_nohup.rs index ce11a743f38..8abdefea367 100644 --- a/tests/by-util/test_nohup.rs +++ b/tests/by-util/test_nohup.rs @@ -257,11 +257,7 @@ fn test_nohup_file_permissions_ignore_umask_always_o600() { .permissions() .mode() & 0o777; - assert_eq!( - mode, 0o600, - "with umask {:o}, got mode {:o}", - umask_val, mode - ); + assert_eq!(mode, 0o600, "with umask {umask_val:o}, got mode {mode:o}"); } } @@ -333,11 +329,9 @@ fn test_nohup_stderr_write_failure() { } for (posixly_correct, expected_code) in [(false, 125), (true, 127)] { - let dev_full = match OpenOptions::new().write(true).open("/dev/full") { - Ok(f) => f, - Err(_) => return, + let Ok(dev_full) = OpenOptions::new().write(true).open("/dev/full") else { + return; }; - let mut cmd = new_ucmd!(); if posixly_correct { cmd.env("POSIXLY_CORRECT", "1"); From 521b06ff14f2452e66f65ebe83f83cd49e61eb40 Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Sun, 14 Dec 2025 19:09:54 +0000 Subject: [PATCH 4/5] nohup: use ExitCode instead of custom error with no message --- src/uu/nohup/src/nohup.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/uu/nohup/src/nohup.rs b/src/uu/nohup/src/nohup.rs index a652fd6b65c..1deb3f42d43 100644 --- a/src/uu/nohup/src/nohup.rs +++ b/src/uu/nohup/src/nohup.rs @@ -18,7 +18,7 @@ use std::path::{Path, PathBuf}; use std::process; use thiserror::Error; use uucore::display::Quotable; -use uucore::error::{UError, UResult, set_exit_code}; +use uucore::error::{ExitCode, UError, UResult, set_exit_code}; use uucore::format_usage; use uucore::translate; @@ -46,17 +46,12 @@ enum NohupError { #[error("{}", translate!("nohup-error-open-failed-both", "first_path" => NOHUP_OUT.quote(), "first_err" => _1, "second_path" => _2.quote(), "second_err" => _3))] OpenFailed2(i32, #[source] Error, String, Error), - - #[error("")] - StderrWriteFailed(i32), } impl UError for NohupError { fn code(&self) -> i32 { match self { - Self::OpenFailed(code, _) - | Self::OpenFailed2(code, _, _, _) - | Self::StderrWriteFailed(code) => *code, + Self::OpenFailed(code, _) | Self::OpenFailed2(code, _, _, _) => *code, _ => 2, } } @@ -71,10 +66,11 @@ fn failure_code() -> i32 { /// We are unable to use the regular show_error because we need to detect if stderr /// is unavailable because GNU nohup exits with 125 if it can't write to stderr. +/// When stderr is unavailable, we use ExitCode to exit silently with the appropriate code. fn write_stderr(msg: &str) -> UResult<()> { let mut stderr = std::io::stderr(); if writeln!(stderr, "nohup: {msg}").is_err() || stderr.flush().is_err() { - return Err(NohupError::StderrWriteFailed(failure_code()).into()); + return Err(ExitCode(failure_code()).into()); } Ok(()) } From 0aeea3b699f993a2336b2439f0daad6f9dcbae12 Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Wed, 17 Dec 2025 15:28:47 +0000 Subject: [PATCH 5/5] nohup: address review comments - use to_owned and to_string_lossy --- src/uu/nohup/src/nohup.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/nohup/src/nohup.rs b/src/uu/nohup/src/nohup.rs index 1deb3f42d43..afcc5b6e874 100644 --- a/src/uu/nohup/src/nohup.rs +++ b/src/uu/nohup/src/nohup.rs @@ -185,14 +185,14 @@ fn find_stdout() -> UResult<(File, String)> { let exit_code = failure_code(); match open_nohup_file(Path::new(NOHUP_OUT)) { - Ok(t) => Ok((t, NOHUP_OUT.to_string())), + Ok(t) => Ok((t, NOHUP_OUT.to_owned())), Err(e1) => { let Ok(home) = env::var("HOME") else { return Err(NohupError::OpenFailed(exit_code, e1).into()); }; let mut homeout = PathBuf::from(home); homeout.push(NOHUP_OUT); - let homeout_str = homeout.to_str().unwrap().to_string(); + let homeout_str = homeout.to_string_lossy().into_owned(); match open_nohup_file(&homeout) { Ok(t) => Ok((t, homeout_str)), Err(e2) => Err(NohupError::OpenFailed2(exit_code, e1, homeout_str, e2).into()),