Skip to content

Commit 57e2c06

Browse files
committed
Auto merge of #101077 - sunshowers:signal-mask-inherit, r=sunshowers
Change process spawning to inherit the parent's signal mask by default Previously, the signal mask was always reset when a child process is started. This breaks tools like `nohup` which expect `SIGHUP` to be blocked for all transitive processes. 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.
2 parents ba9d01b + a52c79e commit 57e2c06

File tree

8 files changed

+143
-79
lines changed

8 files changed

+143
-79
lines changed

compiler/rustc_session/src/config/sigpipe.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
//! NOTE: Keep these constants in sync with `library/std/src/sys/unix/mod.rs`!
22
3+
/// The default value if `#[unix_sigpipe]` is not specified. This resolves
4+
/// to `SIG_IGN` in `library/std/src/sys/unix/mod.rs`.
5+
///
6+
/// Note that `SIG_IGN` has been the Rust default since 2014. See
7+
/// <https://github.com/rust-lang/rust/issues/62569>.
8+
#[allow(dead_code)]
9+
pub const DEFAULT: u8 = 0;
10+
311
/// Do not touch `SIGPIPE`. Use whatever the parent process uses.
412
#[allow(dead_code)]
513
pub const INHERIT: u8 = 1;
@@ -15,8 +23,3 @@ pub const SIG_IGN: u8 = 2;
1523
/// such as `head -n 1`.
1624
#[allow(dead_code)]
1725
pub const SIG_DFL: u8 = 3;
18-
19-
/// `SIG_IGN` has been the Rust default since 2014. See
20-
/// <https://github.com/rust-lang/rust/issues/62569>.
21-
#[allow(dead_code)]
22-
pub const DEFAULT: u8 = SIG_IGN;

library/std/src/rt.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ macro_rules! rtunwrap {
8989
// `src/tools/tidy/src/pal.rs` for more info. On all other platforms, `sigpipe`
9090
// has a value, but its value is ignored.
9191
//
92-
// Even though it is an `u8`, it only ever has 3 values. These are documented in
92+
// Even though it is an `u8`, it only ever has 4 values. These are documented in
9393
// `compiler/rustc_session/src/config/sigpipe.rs`.
9494
#[cfg_attr(test, allow(dead_code))]
9595
unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) {

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

+34-4
Original file line numberDiff line numberDiff line change
@@ -163,24 +163,54 @@ pub unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) {
163163
// See the other file for docs. NOTE: Make sure to keep them in
164164
// sync!
165165
mod sigpipe {
166+
pub const DEFAULT: u8 = 0;
166167
pub const INHERIT: u8 = 1;
167168
pub const SIG_IGN: u8 = 2;
168169
pub const SIG_DFL: u8 = 3;
169170
}
170171

171-
let handler = match sigpipe {
172-
sigpipe::INHERIT => None,
173-
sigpipe::SIG_IGN => Some(libc::SIG_IGN),
174-
sigpipe::SIG_DFL => Some(libc::SIG_DFL),
172+
let (sigpipe_attr_specified, handler) = match sigpipe {
173+
sigpipe::DEFAULT => (false, Some(libc::SIG_IGN)),
174+
sigpipe::INHERIT => (true, None),
175+
sigpipe::SIG_IGN => (true, Some(libc::SIG_IGN)),
176+
sigpipe::SIG_DFL => (true, Some(libc::SIG_DFL)),
175177
_ => unreachable!(),
176178
};
179+
// The bootstrap compiler doesn't know about sigpipe::DEFAULT, and always passes in
180+
// SIG_IGN. This causes some tests to fail because they expect SIGPIPE to be reset to
181+
// default on process spawning (which doesn't happen if #[unix_sigpipe] is specified).
182+
// Since we can't differentiate between the cases here, treat SIG_IGN as DEFAULT
183+
// unconditionally.
184+
if sigpipe_attr_specified && !(cfg!(bootstrap) && sigpipe == sigpipe::SIG_IGN) {
185+
UNIX_SIGPIPE_ATTR_SPECIFIED.store(true, crate::sync::atomic::Ordering::Relaxed);
186+
}
177187
if let Some(handler) = handler {
178188
rtassert!(signal(libc::SIGPIPE, handler) != libc::SIG_ERR);
179189
}
180190
}
181191
}
182192
}
183193

194+
// This is set (up to once) in reset_sigpipe.
195+
#[cfg(not(any(
196+
target_os = "espidf",
197+
target_os = "emscripten",
198+
target_os = "fuchsia",
199+
target_os = "horizon"
200+
)))]
201+
static UNIX_SIGPIPE_ATTR_SPECIFIED: crate::sync::atomic::AtomicBool =
202+
crate::sync::atomic::AtomicBool::new(false);
203+
204+
#[cfg(not(any(
205+
target_os = "espidf",
206+
target_os = "emscripten",
207+
target_os = "fuchsia",
208+
target_os = "horizon"
209+
)))]
210+
pub(crate) fn unix_sigpipe_attr_specified() -> bool {
211+
UNIX_SIGPIPE_ATTR_SPECIFIED.load(crate::sync::atomic::Ordering::Relaxed)
212+
}
213+
184214
// SAFETY: must be called only once during runtime cleanup.
185215
// NOTE: this is not guaranteed to run, for example when the program aborts.
186216
pub unsafe fn cleanup() {

library/std/src/sys/unix/process/process_common.rs

+2
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,12 @@ cfg_if::cfg_if! {
3939
// https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h
4040
cfg_if::cfg_if! {
4141
if #[cfg(target_os = "android")] {
42+
#[allow(dead_code)]
4243
pub unsafe fn sigemptyset(set: *mut libc::sigset_t) -> libc::c_int {
4344
set.write_bytes(0u8, 1);
4445
return 0;
4546
}
47+
4648
#[allow(dead_code)]
4749
pub unsafe fn sigaddset(set: *mut libc::sigset_t, signum: libc::c_int) -> libc::c_int {
4850
use crate::{

library/std/src/sys/unix/process/process_common/tests.rs

+46-33
Original file line numberDiff line numberDiff line change
@@ -31,41 +31,54 @@ macro_rules! t {
3131
ignore
3232
)]
3333
fn test_process_mask() {
34-
unsafe {
35-
// Test to make sure that a signal mask does not get inherited.
36-
let mut cmd = Command::new(OsStr::new("cat"));
37-
38-
let mut set = mem::MaybeUninit::<libc::sigset_t>::uninit();
39-
let mut old_set = mem::MaybeUninit::<libc::sigset_t>::uninit();
40-
t!(cvt(sigemptyset(set.as_mut_ptr())));
41-
t!(cvt(sigaddset(set.as_mut_ptr(), libc::SIGINT)));
42-
t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, set.as_ptr(), old_set.as_mut_ptr())));
43-
44-
cmd.stdin(Stdio::MakePipe);
45-
cmd.stdout(Stdio::MakePipe);
46-
47-
let (mut cat, mut pipes) = t!(cmd.spawn(Stdio::Null, true));
48-
let stdin_write = pipes.stdin.take().unwrap();
49-
let stdout_read = pipes.stdout.take().unwrap();
50-
51-
t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, old_set.as_ptr(), ptr::null_mut())));
52-
53-
t!(cvt(libc::kill(cat.id() as libc::pid_t, libc::SIGINT)));
54-
// We need to wait until SIGINT is definitely delivered. The
55-
// easiest way is to write something to cat, and try to read it
56-
// back: if SIGINT is unmasked, it'll get delivered when cat is
57-
// next scheduled.
58-
let _ = stdin_write.write(b"Hello");
59-
drop(stdin_write);
60-
61-
// Either EOF or failure (EPIPE) is okay.
62-
let mut buf = [0; 5];
63-
if let Ok(ret) = stdout_read.read(&mut buf) {
64-
assert_eq!(ret, 0);
34+
// Test to make sure that a signal mask *does* get inherited.
35+
fn test_inner(mut cmd: Command) {
36+
unsafe {
37+
let mut set = mem::MaybeUninit::<libc::sigset_t>::uninit();
38+
let mut old_set = mem::MaybeUninit::<libc::sigset_t>::uninit();
39+
t!(cvt(sigemptyset(set.as_mut_ptr())));
40+
t!(cvt(sigaddset(set.as_mut_ptr(), libc::SIGINT)));
41+
t!(cvt_nz(libc::pthread_sigmask(
42+
libc::SIG_SETMASK,
43+
set.as_ptr(),
44+
old_set.as_mut_ptr()
45+
)));
46+
47+
cmd.stdin(Stdio::MakePipe);
48+
cmd.stdout(Stdio::MakePipe);
49+
50+
let (mut cat, mut pipes) = t!(cmd.spawn(Stdio::Null, true));
51+
let stdin_write = pipes.stdin.take().unwrap();
52+
let stdout_read = pipes.stdout.take().unwrap();
53+
54+
t!(cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, old_set.as_ptr(), ptr::null_mut())));
55+
56+
t!(cvt(libc::kill(cat.id() as libc::pid_t, libc::SIGINT)));
57+
// We need to wait until SIGINT is definitely delivered. The
58+
// easiest way is to write something to cat, and try to read it
59+
// back: if SIGINT is unmasked, it'll get delivered when cat is
60+
// next scheduled.
61+
let _ = stdin_write.write(b"Hello");
62+
drop(stdin_write);
63+
64+
// Exactly 5 bytes should be read.
65+
let mut buf = [0; 5];
66+
let ret = t!(stdout_read.read(&mut buf));
67+
assert_eq!(ret, 5);
68+
assert_eq!(&buf, b"Hello");
69+
70+
t!(cat.wait());
6571
}
66-
67-
t!(cat.wait());
6872
}
73+
74+
// A plain `Command::new` uses the posix_spawn path on many platforms.
75+
let cmd = Command::new(OsStr::new("cat"));
76+
test_inner(cmd);
77+
78+
// Specifying `pre_exec` forces the fork/exec path.
79+
let mut cmd = Command::new(OsStr::new("cat"));
80+
unsafe { cmd.pre_exec(Box::new(|| Ok(()))) };
81+
test_inner(cmd);
6982
}
7083

7184
#[test]

library/std/src/sys/unix/process/process_unix.rs

+39-33
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ use crate::fmt;
22
use crate::io::{self, Error, ErrorKind};
33
use crate::mem;
44
use crate::num::NonZeroI32;
5-
use crate::ptr;
65
use crate::sys;
76
use crate::sys::cvt;
87
use crate::sys::process::process_common::*;
@@ -310,7 +309,7 @@ impl Command {
310309
//FIXME: Redox kernel does not support setgroups yet
311310
#[cfg(not(target_os = "redox"))]
312311
if libc::getuid() == 0 && self.get_groups().is_none() {
313-
cvt(libc::setgroups(0, ptr::null()))?;
312+
cvt(libc::setgroups(0, crate::ptr::null()))?;
314313
}
315314
cvt(libc::setuid(u as uid_t))?;
316315
}
@@ -326,30 +325,26 @@ impl Command {
326325
// emscripten has no signal support.
327326
#[cfg(not(target_os = "emscripten"))]
328327
{
329-
use crate::mem::MaybeUninit;
330-
use crate::sys::cvt_nz;
331-
// Reset signal handling so the child process starts in a
332-
// standardized state. libstd ignores SIGPIPE, and signal-handling
333-
// libraries often set a mask. Child processes inherit ignored
334-
// signals and the signal mask from their parent, but most
335-
// UNIX programs do not reset these things on their own, so we
336-
// need to clean things up now to avoid confusing the program
337-
// we're about to run.
338-
let mut set = MaybeUninit::<libc::sigset_t>::uninit();
339-
cvt(sigemptyset(set.as_mut_ptr()))?;
340-
cvt_nz(libc::pthread_sigmask(libc::SIG_SETMASK, set.as_ptr(), ptr::null_mut()))?;
341-
342-
#[cfg(target_os = "android")] // see issue #88585
343-
{
344-
let mut action: libc::sigaction = mem::zeroed();
345-
action.sa_sigaction = libc::SIG_DFL;
346-
cvt(libc::sigaction(libc::SIGPIPE, &action, ptr::null_mut()))?;
347-
}
348-
#[cfg(not(target_os = "android"))]
349-
{
350-
let ret = sys::signal(libc::SIGPIPE, libc::SIG_DFL);
351-
if ret == libc::SIG_ERR {
352-
return Err(io::Error::last_os_error());
328+
// Inherit the signal mask from the parent rather than resetting it (i.e. do not call
329+
// pthread_sigmask).
330+
331+
// If #[unix_sigpipe] is specified, don't reset SIGPIPE to SIG_DFL.
332+
// If #[unix_sigpipe] is not specified, reset SIGPIPE to SIG_DFL for backward compatibility.
333+
//
334+
// #[unix_sigpipe] is an opportunity to change the default here.
335+
if !crate::sys::unix_sigpipe_attr_specified() {
336+
#[cfg(target_os = "android")] // see issue #88585
337+
{
338+
let mut action: libc::sigaction = mem::zeroed();
339+
action.sa_sigaction = libc::SIG_DFL;
340+
cvt(libc::sigaction(libc::SIGPIPE, &action, crate::ptr::null_mut()))?;
341+
}
342+
#[cfg(not(target_os = "android"))]
343+
{
344+
let ret = sys::signal(libc::SIGPIPE, libc::SIG_DFL);
345+
if ret == libc::SIG_ERR {
346+
return Err(io::Error::last_os_error());
347+
}
353348
}
354349
}
355350
}
@@ -411,7 +406,7 @@ impl Command {
411406
envp: Option<&CStringArray>,
412407
) -> io::Result<Option<Process>> {
413408
use crate::mem::MaybeUninit;
414-
use crate::sys::{self, cvt_nz};
409+
use crate::sys::{self, cvt_nz, unix_sigpipe_attr_specified};
415410

416411
if self.get_gid().is_some()
417412
|| self.get_uid().is_some()
@@ -531,13 +526,24 @@ impl Command {
531526
cvt_nz(libc::posix_spawnattr_setpgroup(attrs.0.as_mut_ptr(), pgroup))?;
532527
}
533528

534-
let mut set = MaybeUninit::<libc::sigset_t>::uninit();
535-
cvt(sigemptyset(set.as_mut_ptr()))?;
536-
cvt_nz(libc::posix_spawnattr_setsigmask(attrs.0.as_mut_ptr(), set.as_ptr()))?;
537-
cvt(sigaddset(set.as_mut_ptr(), libc::SIGPIPE))?;
538-
cvt_nz(libc::posix_spawnattr_setsigdefault(attrs.0.as_mut_ptr(), set.as_ptr()))?;
529+
// Inherit the signal mask from this process rather than resetting it (i.e. do not call
530+
// posix_spawnattr_setsigmask).
531+
532+
// If #[unix_sigpipe] is specified, don't reset SIGPIPE to SIG_DFL.
533+
// If #[unix_sigpipe] is not specified, reset SIGPIPE to SIG_DFL for backward compatibility.
534+
//
535+
// #[unix_sigpipe] is an opportunity to change the default here.
536+
if !unix_sigpipe_attr_specified() {
537+
let mut default_set = MaybeUninit::<libc::sigset_t>::uninit();
538+
cvt(sigemptyset(default_set.as_mut_ptr()))?;
539+
cvt(sigaddset(default_set.as_mut_ptr(), libc::SIGPIPE))?;
540+
cvt_nz(libc::posix_spawnattr_setsigdefault(
541+
attrs.0.as_mut_ptr(),
542+
default_set.as_ptr(),
543+
))?;
544+
flags |= libc::POSIX_SPAWN_SETSIGDEF;
545+
}
539546

540-
flags |= libc::POSIX_SPAWN_SETSIGDEF | libc::POSIX_SPAWN_SETSIGMASK;
541547
cvt_nz(libc::posix_spawnattr_setflags(attrs.0.as_mut_ptr(), flags as _))?;
542548

543549
// Make sure we synchronize access to the global `environ` resource

src/doc/unstable-book/src/language-features/unix-sigpipe.md

+9-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ hello world
3636

3737
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.
3838

39-
This is what libstd has done by default since 2014. Omitting `#[unix_sigpipe = "..."]` is the same as having `#[unix_sigpipe = "sig_ign"]`.
39+
This is what libstd has done by default since 2014. (However, see the note on child processes below.)
4040

4141
### Example
4242

@@ -52,3 +52,11 @@ hello world
5252
thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9
5353
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
5454
```
55+
56+
### Note on child processes
57+
58+
When spawning child processes, the legacy Rust behavior if `#[unix_sigpipe]` is not specified is to
59+
reset `SIGPIPE` to `SIG_DFL`.
60+
61+
If `#[unix_sigpipe = "..."]` is specified, no matter what its value is, the signal disposition of
62+
`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

+4-2
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ pub fn assert_sigpipe_handler(expected_handler: SignalHandler) {
2323
SignalHandler::Ignore => libc::SIG_IGN,
2424
SignalHandler::Default => libc::SIG_DFL,
2525
};
26-
assert_eq!(prev, expected);
26+
assert_eq!(prev, expected, "expected sigpipe value matches actual value");
2727

2828
// Unlikely to matter, but restore the old value anyway
29-
unsafe { libc::signal(libc::SIGPIPE, prev); };
29+
unsafe {
30+
libc::signal(libc::SIGPIPE, prev);
31+
};
3032
}
3133
}

0 commit comments

Comments
 (0)