From 820123a949705f404ff080759c32dba4a4d89580 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 20 Apr 2021 20:04:31 +0100 Subject: panic/fork: Command: Do not unwind after fork() in child Unwinding after fork() in the child is UB on some platforms, because on those (including musl) malloc can be UB in the child of a multithreaded program, and unwinding must box for the payload. Even if it's safe, unwinding past fork() in the child causes whatever traps the unwind to return twice. This is very strange and clearly not desirable. With the default behaviour of the thread library, this can even result in a panic in the child being transformed into zero exit status (ie, success) as seen in the parent! Fixes #79740. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'library/std/src/sys') diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index ed9044382a8..08b500b9c82 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -54,6 +54,7 @@ impl Command { let (env_lock, pid) = unsafe { (sys::os::env_read_lock(), cvt(libc::fork())?) }; if pid == 0 { + crate::panic::always_abort(); mem::forget(env_lock); drop(input); let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref()) }; -- cgit 1.4.1-3-g733a5 From f8015061c88ba35f7af09ff68a054ffe6c87990c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 11:42:44 +0000 Subject: panic tests: Command: Test that we do not unwind past fork This is safe (does not involve heap allocation) but we don't yet have a test to ensure that stays true. That will come in a moment. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- .../std/src/sys/unix/process/process_unix/tests.rs | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'library/std/src/sys') diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 02c469fbcdf..61b2e4a145f 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -1,3 +1,7 @@ +use crate::os::unix::process::{CommandExt, ExitStatusExt}; +use crate::panic::catch_unwind; +use crate::process::Command; + #[test] fn exitstatus_display_tests() { // In practice this is the same on every Unix. @@ -28,3 +32,22 @@ fn exitstatus_display_tests() { t(0x000ff, "unrecognised wait status: 255 0xff"); } } + +#[test] +fn test_command_fork_no_unwind() { + let got = catch_unwind(|| { + let mut c = Command::new("echo"); + c.arg("hi"); + unsafe { + c.pre_exec(|| panic!("{}", "crash now!")); + } + let st = c.status().expect("failed to get command status"); + dbg!(st); + st + }); + dbg!(&got); + let status = got.expect("panic unexpectedly propagated"); + dbg!(status); + let signal = status.signal().expect("expected child process to die of signal"); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL); +} -- cgit 1.4.1-3-g733a5 From a17eab7beddf87807d9d7fc71b7dfb90b5e2488a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 12:16:11 +0000 Subject: panic ui test: Provide comprehensive test for panic after fork This tests that we can indeed safely panic after fork, both a raw libc::fork and in a Command pre_exec hook. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- .../std/src/sys/unix/process/process_unix/tests.rs | 3 + src/test/ui/panics/abort-on-panic.rs | 48 ++++--- src/test/ui/process/process-panic-after-fork.rs | 150 +++++++++++++++++++++ 3 files changed, 179 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/process/process-panic-after-fork.rs (limited to 'library/std/src/sys') diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 61b2e4a145f..59953a2230f 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -2,6 +2,9 @@ use crate::os::unix::process::{CommandExt, ExitStatusExt}; use crate::panic::catch_unwind; use crate::process::Command; +// Many of the other aspects of this situation, including heap alloc concurrency +// safety etc., are tested in src/test/ui/process/process-panic-after-fork.rs + #[test] fn exitstatus_display_tests() { // In practice this is the same on every Unix. diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index f34cf6a9cbf..3ef6d5d1874 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -23,41 +23,45 @@ extern "Rust" fn panic_in_rust_abi() { panic!("TestRust"); } -fn test() { - let _ = panic::catch_unwind(|| { panic_in_ffi(); }); - // The process should have aborted by now. +fn should_have_aborted() { io::stdout().write(b"This should never be printed.\n"); let _ = io::stdout().flush(); } +fn test() { + let _ = panic::catch_unwind(|| { panic_in_ffi(); }); + should_have_aborted(); +} + fn testrust() { let _ = panic::catch_unwind(|| { panic_in_rust_abi(); }); - // The process should have aborted by now. - io::stdout().write(b"This should never be printed.\n"); - let _ = io::stdout().flush(); + should_have_aborted(); } fn main() { + let tests: &[(_, fn())] = &[ + ("test", test), + ("testrust", testrust), + ]; + let args: Vec = env::args().collect(); if args.len() > 1 { // This is inside the self-executed command. - match &*args[1] { - "test" => return test(), - "testrust" => return testrust(), - _ => panic!("bad test"), + for (a,f) in tests { + if &args[1] == a { return f() } } + panic!("bad test"); } - // These end up calling the self-execution branches above. - let mut p = Command::new(&args[0]) - .stdout(Stdio::piped()) - .stdin(Stdio::piped()) - .arg("test").spawn().unwrap(); - assert!(!p.wait().unwrap().success()); - - let mut p = Command::new(&args[0]) - .stdout(Stdio::piped()) - .stdin(Stdio::piped()) - .arg("testrust").spawn().unwrap(); - assert!(!p.wait().unwrap().success()); + let execute_self_expecting_abort = |arg| { + let mut p = Command::new(&args[0]) + .stdout(Stdio::piped()) + .stdin(Stdio::piped()) + .arg(arg).spawn().unwrap(); + assert!(!p.wait().unwrap().success()); + }; + + for (a,_f) in tests { + execute_self_expecting_abort(a); + } } diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs new file mode 100644 index 00000000000..6e07a1611c5 --- /dev/null +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -0,0 +1,150 @@ +// run-pass +// no-prefer-dynamic +// ignore-wasm32-bare no libc +// ignore-windows +// ignore-sgx no libc +// ignore-emscripten no processes +// ignore-sgx no processes + +#![feature(bench_black_box)] +#![feature(rustc_private)] +#![feature(never_type)] +#![feature(panic_always_abort)] + +extern crate libc; + +use std::alloc::{GlobalAlloc, Layout}; +use std::fmt; +use std::panic::{self, panic_any}; +use std::os::unix::process::{CommandExt, ExitStatusExt}; +use std::process::{self, Command, ExitStatus}; +use std::sync::atomic::{AtomicU32, Ordering}; + +use libc::c_int; + +/// This stunt allocator allows us to spot heap allocations in the child. +struct PidChecking { + parent: A, + require_pid: AtomicU32, +} + +#[global_allocator] +static ALLOCATOR: PidChecking = PidChecking { + parent: std::alloc::System, + require_pid: AtomicU32::new(0), +}; + +impl PidChecking { + fn engage(&self) { + let parent_pid = process::id(); + eprintln!("engaging allocator trap, parent pid={}", parent_pid); + self.require_pid.store(parent_pid, Ordering::Release); + } + fn check(&self) { + let require_pid = self.require_pid.load(Ordering::Acquire); + if require_pid != 0 { + let actual_pid = process::id(); + if require_pid != actual_pid { + unsafe { + libc::raise(libc::SIGTRAP); + } + } + } + } +} + +unsafe impl GlobalAlloc for PidChecking { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + self.check(); + self.parent.alloc(layout) + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + self.check(); + self.parent.dealloc(ptr, layout) + } + + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + self.check(); + self.parent.alloc_zeroed(layout) + } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + self.check(); + self.parent.realloc(ptr, layout, new_size) + } +} + +fn expect_aborted(status: ExitStatus) { + dbg!(status); + let signal = status.signal().expect("expected child process to die of signal"); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL); +} + +fn main() { + ALLOCATOR.engage(); + + fn run(do_panic: &dyn Fn()) -> ExitStatus { + let child = unsafe { libc::fork() }; + assert!(child >= 0); + if child == 0 { + panic::always_abort(); + do_panic(); + process::exit(0); + } + let mut status: c_int = 0; + let got = unsafe { libc::waitpid(child, &mut status, 0) }; + assert_eq!(got, child); + let status = ExitStatus::from_raw(status.into()); + status + } + + fn one(do_panic: &dyn Fn()) { + let status = run(do_panic); + expect_aborted(status); + } + + one(&|| panic!()); + one(&|| panic!("some message")); + one(&|| panic!("message with argument: {}", 42)); + + #[derive(Debug)] + struct Wotsit { } + one(&|| panic_any(Wotsit { })); + + let mut c = Command::new("echo"); + unsafe { + c.pre_exec(|| panic!("{}", "crash now!")); + } + let st = c.status().expect("failed to get command status"); + expect_aborted(st); + + struct DisplayWithHeap; + impl fmt::Display for DisplayWithHeap { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { + let s = vec![0; 100]; + let s = std::hint::black_box(s); + write!(f, "{:?}", s) + } + } + + // Some panics in the stdlib that we want not to allocate, as + // otherwise these facilities become impossible to use in the + // child after fork, which is really quite awkward. + + one(&|| { None::.unwrap(); }); + one(&|| { None::.expect("unwrapped a none"); }); + one(&|| { std::str::from_utf8(b"\xff").unwrap(); }); + one(&|| { + let x = [0, 1, 2, 3]; + let y = x[std::hint::black_box(4)]; + let _z = std::hint::black_box(y); + }); + + // Finally, check that our stunt allocator can actually catch an allocation after fork. + // ie, that our test is effective. + + let status = run(&|| panic!("allocating to display... {}", DisplayWithHeap)); + dbg!(status); + assert_eq!(status.signal(), Some(libc::SIGTRAP)); +} -- cgit 1.4.1-3-g733a5 From 8220f2f2127b9aec972163ded97be7d8cff6b9a8 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 7 May 2021 16:15:53 +0100 Subject: panic/fork test: Do not run on emscripten fork fails there. The failure message is confusing: so c.status() returns an Err, the closure panics, and the test thinks the panic was propagated from inside the child. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- library/std/src/sys/unix/process/process_unix/tests.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'library/std/src/sys') diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 59953a2230f..d7915d81ebd 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -37,6 +37,7 @@ fn exitstatus_display_tests() { } #[test] +#[cfg_attr(target_os = "emscripten", ignore)] fn test_command_fork_no_unwind() { let got = catch_unwind(|| { let mut c = Command::new("echo"); -- cgit 1.4.1-3-g733a5 From 6369637a192bbd0a2fbf8084345ddb7c099aa460 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 13 May 2021 18:41:18 +0100 Subject: Tolerate SIGTRAP for panic abort after panic::always_abort Some platforma (eg ARM64) apparently generate SIGTRAP for panic abort! See eg https://github.com/rust-lang/rust/pull/81858#issuecomment-840702765 This is probably a bug, but we don't want to entangle this MR with it. When it's fixed, this commit should be reverted. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix/tests.rs | 2 +- src/test/ui/process/process-panic-after-fork.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'library/std/src/sys') diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index d7915d81ebd..157debf2d25 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -53,5 +53,5 @@ fn test_command_fork_no_unwind() { let status = got.expect("panic unexpectedly propagated"); dbg!(status); let signal = status.signal().expect("expected child process to die of signal"); - assert!(signal == libc::SIGABRT || signal == libc::SIGILL); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL || signal == libc::SIGTRAP); } diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index 5cc436c335e..f178baefcf6 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -78,7 +78,7 @@ unsafe impl GlobalAlloc for PidChecking { fn expect_aborted(status: ExitStatus) { dbg!(status); let signal = status.signal().expect("expected child process to die of signal"); - assert!(signal == libc::SIGABRT || signal == libc::SIGILL); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL || signal == libc::SIGTRAP); } fn main() { -- cgit 1.4.1-3-g733a5