Change process spawning to inherit the parent's signal mask by default

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
+/// <https://github.com/rust-lang/rust/issues/62569>.
+#[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
-/// <https://github.com/rust-lang/rust/issues/62569>.
-#[allow(dead_code)]
-pub const DEFAULT: u8 = SIG_IGN;

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) {

library/std/src/sys/unix/mod.rs

@@ -163,24 +163,54 @@ 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);
             }
         }
     }
 }
 
+// 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() {

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::{

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::<libc::sigset_t>::uninit();
-        let mut old_set = mem::MaybeUninit::<libc::sigset_t>::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::<libc::sigset_t>::uninit();
+            let mut old_set = mem::MaybeUninit::<libc::sigset_t>::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]

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::<libc::sigset_t>::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<Option<Process>> {
         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::<libc::sigset_t>::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::<libc::sigset_t>::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

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.

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);
+        };
     }
 }