From a52c79e859142c1cd5c0c5bdb73f16b754e1b98f Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 22 Sep 2022 22:48:14 -0700 Subject: [PATCH] Change process spawning to inherit the parent's signal mask by default Previously, the signal mask is always reset when a child process is started. This breaks tools like `nohup` which expect `SIGHUP` to be blocked. With this change, the default behavior changes to inherit the signal mask. This also changes the signal disposition for `SIGPIPE` to only be changed if the `#[unix_sigpipe]` attribute isn't set. --- compiler/rustc_session/src/config/sigpipe.rs | 13 +-- library/std/src/rt.rs | 2 +- library/std/src/sys/unix/mod.rs | 38 ++++++++- .../src/sys/unix/process/process_common.rs | 2 + .../sys/unix/process/process_common/tests.rs | 79 +++++++++++-------- .../std/src/sys/unix/process/process_unix.rs | 72 +++++++++-------- .../src/language-features/unix-sigpipe.md | 10 ++- .../unix_sigpipe/auxiliary/sigpipe-utils.rs | 6 +- 8 files changed, 143 insertions(+), 79 deletions(-) diff --git a/compiler/rustc_session/src/config/sigpipe.rs b/compiler/rustc_session/src/config/sigpipe.rs index a5c94118a47e2..53692ad7cc92b 100644 --- a/compiler/rustc_session/src/config/sigpipe.rs +++ b/compiler/rustc_session/src/config/sigpipe.rs @@ -1,5 +1,13 @@ //! NOTE: Keep these constants in sync with `library/std/src/sys/unix/mod.rs`! +/// The default value if `#[unix_sigpipe]` is not specified. This resolves +/// to `SIG_IGN` in `library/std/src/sys/unix/mod.rs`. +/// +/// Note that `SIG_IGN` has been the Rust default since 2014. See +/// . +#[allow(dead_code)] +pub const DEFAULT: u8 = 0; + /// Do not touch `SIGPIPE`. Use whatever the parent process uses. #[allow(dead_code)] pub const INHERIT: u8 = 1; @@ -15,8 +23,3 @@ pub const SIG_IGN: u8 = 2; /// such as `head -n 1`. #[allow(dead_code)] pub const SIG_DFL: u8 = 3; - -/// `SIG_IGN` has been the Rust default since 2014. See -/// . -#[allow(dead_code)] -pub const DEFAULT: u8 = SIG_IGN; diff --git a/library/std/src/rt.rs b/library/std/src/rt.rs index b8bcdbece0af3..9c2f0c1dd3eb6 100644 --- a/library/std/src/rt.rs +++ b/library/std/src/rt.rs @@ -89,7 +89,7 @@ macro_rules! rtunwrap { // `src/tools/tidy/src/pal.rs` for more info. On all other platforms, `sigpipe` // has a value, but its value is ignored. // -// Even though it is an `u8`, it only ever has 3 values. These are documented in +// Even though it is an `u8`, it only ever has 4 values. These are documented in // `compiler/rustc_session/src/config/sigpipe.rs`. #[cfg_attr(test, allow(dead_code))] unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) { diff --git a/library/std/src/sys/unix/mod.rs b/library/std/src/sys/unix/mod.rs index c84e292eac152..4a9f356b63cc1 100644 --- a/library/std/src/sys/unix/mod.rs +++ b/library/std/src/sys/unix/mod.rs @@ -163,17 +163,27 @@ pub unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) { // See the other file for docs. NOTE: Make sure to keep them in // sync! mod sigpipe { + pub const DEFAULT: u8 = 0; pub const INHERIT: u8 = 1; pub const SIG_IGN: u8 = 2; pub const SIG_DFL: u8 = 3; } - let handler = match sigpipe { - sigpipe::INHERIT => None, - sigpipe::SIG_IGN => Some(libc::SIG_IGN), - sigpipe::SIG_DFL => Some(libc::SIG_DFL), + let (sigpipe_attr_specified, handler) = match sigpipe { + sigpipe::DEFAULT => (false, Some(libc::SIG_IGN)), + sigpipe::INHERIT => (true, None), + sigpipe::SIG_IGN => (true, Some(libc::SIG_IGN)), + sigpipe::SIG_DFL => (true, Some(libc::SIG_DFL)), _ => unreachable!(), }; + // The bootstrap compiler doesn't know about sigpipe::DEFAULT, and always passes in + // SIG_IGN. This causes some tests to fail because they expect SIGPIPE to be reset to + // default on process spawning (which doesn't happen if #[unix_sigpipe] is specified). + // Since we can't differentiate between the cases here, treat SIG_IGN as DEFAULT + // unconditionally. + if sigpipe_attr_specified && !(cfg!(bootstrap) && sigpipe == sigpipe::SIG_IGN) { + UNIX_SIGPIPE_ATTR_SPECIFIED.store(true, crate::sync::atomic::Ordering::Relaxed); + } if let Some(handler) = handler { rtassert!(signal(libc::SIGPIPE, handler) != libc::SIG_ERR); } @@ -181,6 +191,26 @@ pub unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) { } } +// This is set (up to once) in reset_sigpipe. +#[cfg(not(any( + target_os = "espidf", + target_os = "emscripten", + target_os = "fuchsia", + target_os = "horizon" +)))] +static UNIX_SIGPIPE_ATTR_SPECIFIED: crate::sync::atomic::AtomicBool = + crate::sync::atomic::AtomicBool::new(false); + +#[cfg(not(any( + target_os = "espidf", + target_os = "emscripten", + target_os = "fuchsia", + target_os = "horizon" +)))] +pub(crate) fn unix_sigpipe_attr_specified() -> bool { + UNIX_SIGPIPE_ATTR_SPECIFIED.load(crate::sync::atomic::Ordering::Relaxed) +} + // SAFETY: must be called only once during runtime cleanup. // NOTE: this is not guaranteed to run, for example when the program aborts. pub unsafe fn cleanup() { diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index 2834ee0ace835..848adca78c076 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -39,10 +39,12 @@ cfg_if::cfg_if! { // https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h cfg_if::cfg_if! { if #[cfg(target_os = "android")] { + #[allow(dead_code)] pub unsafe fn sigemptyset(set: *mut libc::sigset_t) -> libc::c_int { set.write_bytes(0u8, 1); return 0; } + #[allow(dead_code)] pub unsafe fn sigaddset(set: *mut libc::sigset_t, signum: libc::c_int) -> libc::c_int { use crate::{ diff --git a/library/std/src/sys/unix/process/process_common/tests.rs b/library/std/src/sys/unix/process/process_common/tests.rs index d176b3401c03c..03631e4e33bf5 100644 --- a/library/std/src/sys/unix/process/process_common/tests.rs +++ b/library/std/src/sys/unix/process/process_common/tests.rs @@ -31,41 +31,54 @@ macro_rules! t { ignore )] fn test_process_mask() { - unsafe { - // Test to make sure that a signal mask does not get inherited. - let mut cmd = Command::new(OsStr::new("cat")); - - let mut set = mem::MaybeUninit::::uninit(); - let mut old_set = mem::MaybeUninit::::uninit(); - t!(cvt(sigemptyset(set.as_mut_ptr()))); - t!(cvt(sigaddset(set.as_mut_ptr(), libc::SIGINT))); - t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, set.as_ptr(), old_set.as_mut_ptr()))); - - cmd.stdin(Stdio::MakePipe); - cmd.stdout(Stdio::MakePipe); - - let (mut cat, mut pipes) = t!(cmd.spawn(Stdio::Null, true)); - let stdin_write = pipes.stdin.take().unwrap(); - let stdout_read = pipes.stdout.take().unwrap(); - - t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, old_set.as_ptr(), ptr::null_mut()))); - - t!(cvt(libc::kill(cat.id() as libc::pid_t, libc::SIGINT))); - // We need to wait until SIGINT is definitely delivered. The - // easiest way is to write something to cat, and try to read it - // back: if SIGINT is unmasked, it'll get delivered when cat is - // next scheduled. - let _ = stdin_write.write(b"Hello"); - drop(stdin_write); - - // Either EOF or failure (EPIPE) is okay. - let mut buf = [0; 5]; - if let Ok(ret) = stdout_read.read(&mut buf) { - assert_eq!(ret, 0); + // Test to make sure that a signal mask *does* get inherited. + fn test_inner(mut cmd: Command) { + unsafe { + let mut set = mem::MaybeUninit::::uninit(); + let mut old_set = mem::MaybeUninit::::uninit(); + t!(cvt(sigemptyset(set.as_mut_ptr()))); + t!(cvt(sigaddset(set.as_mut_ptr(), libc::SIGINT))); + t!(cvt_nz(libc::pthread_sigmask( + libc::SIG_SETMASK, + set.as_ptr(), + old_set.as_mut_ptr() + ))); + + cmd.stdin(Stdio::MakePipe); + cmd.stdout(Stdio::MakePipe); + + let (mut cat, mut pipes) = t!(cmd.spawn(Stdio::Null, true)); + let stdin_write = pipes.stdin.take().unwrap(); + let stdout_read = pipes.stdout.take().unwrap(); + + t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, old_set.as_ptr(), ptr::null_mut()))); + + t!(cvt(libc::kill(cat.id() as libc::pid_t, libc::SIGINT))); + // We need to wait until SIGINT is definitely delivered. The + // easiest way is to write something to cat, and try to read it + // back: if SIGINT is unmasked, it'll get delivered when cat is + // next scheduled. + let _ = stdin_write.write(b"Hello"); + drop(stdin_write); + + // Exactly 5 bytes should be read. + let mut buf = [0; 5]; + let ret = t!(stdout_read.read(&mut buf)); + assert_eq!(ret, 5); + assert_eq!(&buf, b"Hello"); + + t!(cat.wait()); } - - t!(cat.wait()); } + + // A plain `Command::new` uses the posix_spawn path on many platforms. + let cmd = Command::new(OsStr::new("cat")); + test_inner(cmd); + + // Specifying `pre_exec` forces the fork/exec path. + let mut cmd = Command::new(OsStr::new("cat")); + unsafe { cmd.pre_exec(Box::new(|| Ok(()))) }; + test_inner(cmd); } #[test] diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 2ff8e600f7c52..56a805cef7318 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -2,7 +2,6 @@ use crate::fmt; use crate::io::{self, Error, ErrorKind}; use crate::mem; use crate::num::NonZeroI32; -use crate::ptr; use crate::sys; use crate::sys::cvt; use crate::sys::process::process_common::*; @@ -310,7 +309,7 @@ impl Command { //FIXME: Redox kernel does not support setgroups yet #[cfg(not(target_os = "redox"))] if libc::getuid() == 0 && self.get_groups().is_none() { - cvt(libc::setgroups(0, ptr::null()))?; + cvt(libc::setgroups(0, crate::ptr::null()))?; } cvt(libc::setuid(u as uid_t))?; } @@ -326,30 +325,26 @@ impl Command { // emscripten has no signal support. #[cfg(not(target_os = "emscripten"))] { - use crate::mem::MaybeUninit; - use crate::sys::cvt_nz; - // Reset signal handling so the child process starts in a - // standardized state. libstd ignores SIGPIPE, and signal-handling - // libraries often set a mask. Child processes inherit ignored - // signals and the signal mask from their parent, but most - // UNIX programs do not reset these things on their own, so we - // need to clean things up now to avoid confusing the program - // we're about to run. - let mut set = MaybeUninit::::uninit(); - cvt(sigemptyset(set.as_mut_ptr()))?; - cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, set.as_ptr(), ptr::null_mut()))?; - - #[cfg(target_os = "android")] // see issue #88585 - { - let mut action: libc::sigaction = mem::zeroed(); - action.sa_sigaction = libc::SIG_DFL; - cvt(libc::sigaction(libc::SIGPIPE, &action, ptr::null_mut()))?; - } - #[cfg(not(target_os = "android"))] - { - let ret = sys::signal(libc::SIGPIPE, libc::SIG_DFL); - if ret == libc::SIG_ERR { - return Err(io::Error::last_os_error()); + // Inherit the signal mask from the parent rather than resetting it (i.e. do not call + // pthread_sigmask). + + // If #[unix_sigpipe] is specified, don't reset SIGPIPE to SIG_DFL. + // If #[unix_sigpipe] is not specified, reset SIGPIPE to SIG_DFL for backward compatibility. + // + // #[unix_sigpipe] is an opportunity to change the default here. + if !crate::sys::unix_sigpipe_attr_specified() { + #[cfg(target_os = "android")] // see issue #88585 + { + let mut action: libc::sigaction = mem::zeroed(); + action.sa_sigaction = libc::SIG_DFL; + cvt(libc::sigaction(libc::SIGPIPE, &action, crate::ptr::null_mut()))?; + } + #[cfg(not(target_os = "android"))] + { + let ret = sys::signal(libc::SIGPIPE, libc::SIG_DFL); + if ret == libc::SIG_ERR { + return Err(io::Error::last_os_error()); + } } } } @@ -411,7 +406,7 @@ impl Command { envp: Option<&CStringArray>, ) -> io::Result> { use crate::mem::MaybeUninit; - use crate::sys::{self, cvt_nz}; + use crate::sys::{self, cvt_nz, unix_sigpipe_attr_specified}; if self.get_gid().is_some() || self.get_uid().is_some() @@ -531,13 +526,24 @@ impl Command { cvt_nz(libc::posix_spawnattr_setpgroup(attrs.0.as_mut_ptr(), pgroup))?; } - let mut set = MaybeUninit::::uninit(); - cvt(sigemptyset(set.as_mut_ptr()))?; - cvt_nz(libc::posix_spawnattr_setsigmask(attrs.0.as_mut_ptr(), set.as_ptr()))?; - cvt(sigaddset(set.as_mut_ptr(), libc::SIGPIPE))?; - cvt_nz(libc::posix_spawnattr_setsigdefault(attrs.0.as_mut_ptr(), set.as_ptr()))?; + // Inherit the signal mask from this process rather than resetting it (i.e. do not call + // posix_spawnattr_setsigmask). + + // If #[unix_sigpipe] is specified, don't reset SIGPIPE to SIG_DFL. + // If #[unix_sigpipe] is not specified, reset SIGPIPE to SIG_DFL for backward compatibility. + // + // #[unix_sigpipe] is an opportunity to change the default here. + if !unix_sigpipe_attr_specified() { + let mut default_set = MaybeUninit::::uninit(); + cvt(sigemptyset(default_set.as_mut_ptr()))?; + cvt(sigaddset(default_set.as_mut_ptr(), libc::SIGPIPE))?; + cvt_nz(libc::posix_spawnattr_setsigdefault( + attrs.0.as_mut_ptr(), + default_set.as_ptr(), + ))?; + flags |= libc::POSIX_SPAWN_SETSIGDEF; + } - flags |= libc::POSIX_SPAWN_SETSIGDEF | libc::POSIX_SPAWN_SETSIGMASK; cvt_nz(libc::posix_spawnattr_setflags(attrs.0.as_mut_ptr(), flags as _))?; // Make sure we synchronize access to the global `environ` resource diff --git a/src/doc/unstable-book/src/language-features/unix-sigpipe.md b/src/doc/unstable-book/src/language-features/unix-sigpipe.md index aa39b6eb2886f..7ed6a7de895c1 100644 --- a/src/doc/unstable-book/src/language-features/unix-sigpipe.md +++ b/src/doc/unstable-book/src/language-features/unix-sigpipe.md @@ -36,7 +36,7 @@ hello world Set the `SIGPIPE` handler to `SIG_IGN` before invoking `fn main()`. This will result in `ErrorKind::BrokenPipe` errors if you program tries to write to a closed pipe. This is normally what you want if you for example write socket servers, socket clients, or pipe peers. -This is what libstd has done by default since 2014. Omitting `#[unix_sigpipe = "..."]` is the same as having `#[unix_sigpipe = "sig_ign"]`. +This is what libstd has done by default since 2014. (However, see the note on child processes below.) ### Example @@ -52,3 +52,11 @@ hello world thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` + +### Note on child processes + +When spawning child processes, the legacy Rust behavior if `#[unix_sigpipe]` is not specified is to +reset `SIGPIPE` to `SIG_DFL`. + +If `#[unix_sigpipe = "..."]` is specified, no matter what its value is, the signal disposition of +`SIGPIPE` is no longer reset. This means that the child inherits the parent's `SIGPIPE` behavior. diff --git a/src/test/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs b/src/test/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs index e8b4fe7ae5230..74fbae0350e48 100644 --- a/src/test/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs +++ b/src/test/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs @@ -23,9 +23,11 @@ pub fn assert_sigpipe_handler(expected_handler: SignalHandler) { SignalHandler::Ignore => libc::SIG_IGN, SignalHandler::Default => libc::SIG_DFL, }; - assert_eq!(prev, expected); + assert_eq!(prev, expected, "expected sigpipe value matches actual value"); // Unlikely to matter, but restore the old value anyway - unsafe { libc::signal(libc::SIGPIPE, prev); }; + unsafe { + libc::signal(libc::SIGPIPE, prev); + }; } }