From a9f43a2a8f845ac7be462cc5300da2350995c546 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 14:58:05 +0100 Subject: [PATCH 01/14] std panicking: Make decrease() return () Nothing uses the return value. This will make the next changes easier. Signed-off-by: Ian Jackson --- library/std/src/panicking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 6cd572cbe87c1..fbcfec7c7a9e9 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -252,13 +252,13 @@ pub mod panic_count { }) } - pub fn decrease() -> usize { + pub fn decrease() { GLOBAL_PANIC_COUNT.fetch_sub(1, Ordering::Relaxed); LOCAL_PANIC_COUNT.with(|c| { let next = c.get() - 1; c.set(next); next - }) + }); } pub fn get() -> usize { From 1b1bf2463619e23eba1b36b6d7df276ce73563dd Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 20 Apr 2021 20:02:49 +0100 Subject: [PATCH 02/14] std panicking: Provide panic::always_abort We must change the atomic read on panic entry to `Acquire`, to pick up a possible an `always_panic` on another thread. We add `count` to the names of panic_count::get and ::is_zaero, because now there is another reason why panic ought to maybe abort. Renaming these ensures that we have checked every call site to ensure that they don't need further adjustment. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- library/std/src/panic.rs | 36 ++++++++++++++++++++ library/std/src/panicking.rs | 64 +++++++++++++++++++++++++++--------- 2 files changed, 85 insertions(+), 15 deletions(-) diff --git a/library/std/src/panic.rs b/library/std/src/panic.rs index 3e634239ad301..1c1466492881c 100644 --- a/library/std/src/panic.rs +++ b/library/std/src/panic.rs @@ -461,5 +461,41 @@ pub fn resume_unwind(payload: Box) -> ! { panicking::rust_panic_without_hook(payload) } +/// Make all future panics abort directly without running the panic hook or unwinding. +/// +/// There is no way to undo this; the effect lasts until the process exits or +/// execs (or the equivalent). +/// +/// # Use after fork +/// +/// This function is particularly useful for calling after `libc::fork`. After `fork`, in a +/// multithreaded program it is (on many platforms) not safe to call the allocator. It is also +/// generally highly undesirable for an unwind to unwind past the `fork`, because that results in +/// the unwind propagating to code that was only ever expecting to run in the parent. +/// +/// `panic::always_abort()` helps avoid both of these. It directly avoids any further unwinding, +/// and if there is a panic, the abort will occur without allocating provided that the arguments to +/// panic can be formatted without allocating. +/// +/// Examples +/// +/// ```no_run +/// #![feature(panic_always_abort)] +/// use std::panic; +/// +/// panic::always_abort(); +/// +/// let _ = panic::catch_unwind(|| { +/// panic!("inside the catch"); +/// }); +/// +/// // We will have aborted already, due to the panic. +/// unreachable!(); +/// ``` +#[unstable(feature = "panic_always_abort", issue = "84438")] +pub fn always_abort() { + crate::panicking::panic_count::set_always_abort(); +} + #[cfg(test)] mod tests; diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index fbcfec7c7a9e9..62476581f990f 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -180,7 +180,7 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { fn default_hook(info: &PanicInfo<'_>) { // If this is a double panic, make sure that we print a backtrace // for this panic. Otherwise only print it if logging is enabled. - let backtrace_env = if panic_count::get() >= 2 { + let backtrace_env = if panic_count::get_count() >= 2 { RustBacktrace::Print(crate::backtrace_rs::PrintFmt::Full) } else { backtrace::rust_backtrace_env() @@ -233,6 +233,8 @@ pub mod panic_count { use crate::cell::Cell; use crate::sync::atomic::{AtomicUsize, Ordering}; + pub const ALWAYS_ABORT_FLAG: usize = 1 << (usize::BITS - 1); + // Panic count for the current thread. thread_local! { static LOCAL_PANIC_COUNT: Cell = Cell::new(0) } @@ -241,15 +243,29 @@ pub mod panic_count { // thread, if that thread currently views `GLOBAL_PANIC_COUNT` as being zero, // then `LOCAL_PANIC_COUNT` in that thread is zero. This invariant holds before // and after increase and decrease, but not necessarily during their execution. + // + // Additionally, the top bit of GLOBAL_PANIC_COUNT (GLOBAL_ALWAYS_ABORT_FLAG) + // records whether panic::always_abort() has been called. This can only be + // set, never cleared. + // + // This could be viewed as a struct containing a single bit and an n-1-bit + // value, but if we wrote it like that it would be more than a single word, + // and even a newtype around usize would be clumsy because we need atomics. + // But we use such a tuple for the return type of increase(). + // + // Stealing a bit is fine because it just amounts to assuming that each + // panicking thread consumes at least 2 bytes of address space. static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0); - pub fn increase() -> usize { - GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed); - LOCAL_PANIC_COUNT.with(|c| { - let next = c.get() + 1; - c.set(next); - next - }) + pub fn increase() -> (bool, usize) { + ( + GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Acquire) & ALWAYS_ABORT_FLAG != 0, + LOCAL_PANIC_COUNT.with(|c| { + let next = c.get() + 1; + c.set(next); + next + }), + ) } pub fn decrease() { @@ -261,13 +277,19 @@ pub mod panic_count { }); } - pub fn get() -> usize { + pub fn set_always_abort() { + GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Release); + } + + // Disregards ALWAYS_ABORT_FLAG + pub fn get_count() -> usize { LOCAL_PANIC_COUNT.with(|c| c.get()) } + // Disregards ALWAYS_ABORT_FLAG #[inline] - pub fn is_zero() -> bool { - if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) == 0 { + pub fn count_is_zero() -> bool { + if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) & !ALWAYS_ABORT_FLAG == 0 { // Fast path: if `GLOBAL_PANIC_COUNT` is zero, all threads // (including the current one) will have `LOCAL_PANIC_COUNT` // equal to zero, so TLS access can be avoided. @@ -410,7 +432,7 @@ pub unsafe fn r#try R>(f: F) -> Result> /// Determines whether the current thread is unwinding because of panic. #[inline] pub fn panicking() -> bool { - !panic_count::is_zero() + !panic_count::count_is_zero() } /// The entry point for panicking with a formatted message. @@ -563,15 +585,27 @@ fn rust_panic_with_hook( message: Option<&fmt::Arguments<'_>>, location: &Location<'_>, ) -> ! { - let panics = panic_count::increase(); + let (must_abort, panics) = panic_count::increase(); // If this is the third nested call (e.g., panics == 2, this is 0-indexed), // the panic hook probably triggered the last panic, otherwise the // double-panic check would have aborted the process. In this case abort the // process real quickly as we don't want to try calling it again as it'll // probably just panic again. - if panics > 2 { - util::dumb_print(format_args!("thread panicked while processing panic. aborting.\n")); + if must_abort || panics > 2 { + if panics > 2 { + // Don't try to print the message in this case + // - perhaps that is causing the recursive panics. + util::dumb_print(format_args!("thread panicked while processing panic. aborting.\n")); + } else { + // Unfortunately, this does not print a backtrace, because creating + // a `Backtrace` will allocate, which we must to avoid here. + let panicinfo = PanicInfo::internal_constructor(message, location); + util::dumb_print(format_args!( + "{}\npanicked after panic::always_abort(), aborting.\n", + panicinfo + )); + } intrinsics::abort() } From 3cba120ba453b5371e53766ec816f016d004ed91 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 6 May 2021 15:11:08 +0100 Subject: [PATCH 03/14] std panicking: ALWAYS_ABORT: use Relaxed memory ordering As per https://github.com/rust-lang/rust/pull/81858#discussion_r626507810 Suggested-by: Mara Bos Signed-off-by: Ian Jackson --- library/std/src/panicking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 62476581f990f..a8410bea7342b 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -259,7 +259,7 @@ pub mod panic_count { pub fn increase() -> (bool, usize) { ( - GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Acquire) & ALWAYS_ABORT_FLAG != 0, + GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed) & ALWAYS_ABORT_FLAG != 0, LOCAL_PANIC_COUNT.with(|c| { let next = c.get() + 1; c.set(next); @@ -278,7 +278,7 @@ pub mod panic_count { } pub fn set_always_abort() { - GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Release); + GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Relaxed); } // Disregards ALWAYS_ABORT_FLAG From 820123a949705f404ff080759c32dba4a4d89580 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 20 Apr 2021 20:04:31 +0100 Subject: [PATCH 04/14] 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(+) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index ed9044382a898..08b500b9c825a 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()) }; From 9283cdca362065a215e7f8b460719947493ddc54 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 13:41:49 +0000 Subject: [PATCH 05/14] unix process: pre_exec: Discuss panic safety Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- library/std/src/os/unix/process.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/std/src/os/unix/process.rs b/library/std/src/os/unix/process.rs index 355855bcd10e2..f014a3d7b2539 100644 --- a/library/std/src/os/unix/process.rs +++ b/library/std/src/os/unix/process.rs @@ -75,6 +75,12 @@ pub trait CommandExt: Sealed { /// sure that the closure does not violate library invariants by making /// invalid use of these duplicates. /// + /// Panicking in the closure is safe only if all the format arguments for the + /// panic message can be safely formatted; this is because although + /// `Command` calls [`std::panic::always_abort`](crate::panic::always_abort) + /// before calling the pre_exec hook, panic will still try to format the + /// panic message. + /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these /// locations may not appear where intended. From f8015061c88ba35f7af09ff68a054ffe6c87990c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 11:42:44 +0000 Subject: [PATCH 06/14] 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 --- .../sys/unix/process/process_unix/tests.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) 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 02c469fbcdfd8..61b2e4a145f80 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); +} From a17eab7beddf87807d9d7fc71b7dfb90b5e2488a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 12:16:11 +0000 Subject: [PATCH 07/14] 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 --- .../sys/unix/process/process_unix/tests.rs | 3 + src/test/ui/panics/abort-on-panic.rs | 48 +++--- .../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 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 61b2e4a145f80..59953a2230fce 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 f34cf6a9cbf6d..3ef6d5d187440 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 0000000000000..6e07a1611c5c3 --- /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)); +} From 19429ce132d8cc9526ab4fe46d6287f2ad89ef1c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 15:05:23 +0100 Subject: [PATCH 08/14] panic ui test: Improve error handling Previoously, if somehow this program got a wrong argument, it would panic in the re-executed child. But that looks like a "success" for this program! We mustn't panic unless everything is great. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index 3ef6d5d187440..7cf60ae960212 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -28,6 +28,11 @@ fn should_have_aborted() { let _ = io::stdout().flush(); } +fn bomb_out_but_not_abort(msg: &str) { + eprintln!("bombing out: {}", msg); + exit(1); +} + fn test() { let _ = panic::catch_unwind(|| { panic_in_ffi(); }); should_have_aborted(); @@ -50,7 +55,7 @@ fn main() { for (a,f) in tests { if &args[1] == a { return f() } } - panic!("bad test"); + bomb_out_but_not_abort("bad test"); } let execute_self_expecting_abort = |arg| { @@ -58,7 +63,11 @@ fn main() { .stdout(Stdio::piped()) .stdin(Stdio::piped()) .arg(arg).spawn().unwrap(); - assert!(!p.wait().unwrap().success()); + let status = p.wait().unwrap(); + assert!(!status.success()); + // Any reasonable platform can distinguish a process which + // called exit(1) from one which panicked. + assert_ne!(status.code(), Some(1)); }; for (a,_f) in tests { From 12fe50010da70abfca84ae9c0d6e798e987fa882 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 14:22:42 +0100 Subject: [PATCH 09/14] panic ui test: Add a test for panic::always_abort Our existing tests are only on Unix. We want a general one too. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index 7cf60ae960212..c02552be5192c 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -43,10 +43,17 @@ fn testrust() { should_have_aborted(); } +fn test_always_abort() { + panic::always_abort(); + let _ = panic::catch_unwind(|| { panic!(); }); + should_have_aborted(); +} + fn main() { let tests: &[(_, fn())] = &[ ("test", test), ("testrust", testrust), + ("test_always_abort", test_always_abort), ]; let args: Vec = env::args().collect(); From 756771d54c28c4543d620891e971bf5a95c3ea3e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 15:06:53 +0100 Subject: [PATCH 10/14] panic ui test: Test always_abort on one thread, panic on another This test failed on an earlier version of this branch, where this did not work properly, so I know the test works. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index c02552be5192c..2aea607e95489 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -2,6 +2,7 @@ #![allow(unused_must_use)] #![feature(unwind_attributes)] +#![feature(panic_always_abort)] // Since we mark some ABIs as "nounwind" to LLVM, we must make sure that // we never unwind through them. @@ -11,7 +12,9 @@ use std::{env, panic}; use std::io::prelude::*; use std::io; -use std::process::{Command, Stdio}; +use std::process::{exit, Command, Stdio}; +use std::sync::{Arc, Barrier}; +use std::thread; #[unwind(aborts)] // FIXME(#58794) should work even without the attribute extern "C" fn panic_in_ffi() { @@ -49,11 +52,27 @@ fn test_always_abort() { should_have_aborted(); } +fn test_always_abort_thread() { + let barrier = Arc::new(Barrier::new(2)); + let thr = { + let barrier = barrier.clone(); + thread::spawn(move ||{ + barrier.wait(); + panic!("in thread"); + }) + }; + panic::always_abort(); + barrier.wait(); + let _ = thr.join(); + bomb_out_but_not_abort("joined - but we were supposed to panic!"); +} + fn main() { let tests: &[(_, fn())] = &[ ("test", test), ("testrust", testrust), ("test_always_abort", test_always_abort), + ("test_always_abort_thread", test_always_abort_thread), ]; let args: Vec = env::args().collect(); From 8220f2f2127b9aec972163ded97be7d8cff6b9a8 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 7 May 2021 16:15:53 +0100 Subject: [PATCH 11/14] 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(+) 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 59953a2230fce..d7915d81ebdec 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"); From f6a4963cc86bda5f3611f0f59e1ffe53e4b9f3fa Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 13 May 2021 18:38:25 +0100 Subject: [PATCH 12/14] Use SIGUSR1 rather than SIGTRAP for "allocated after fork" 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 (i) we want to avoid that bug rather than trying to fix it now and (ii) it would better to use a signal that is less at risk of strangeness. I grepped the rust-lang/rut codebase for SIGUSR and there were no hits. Signed-off-by: Ian Jackson --- src/test/ui/process/process-panic-after-fork.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index 6e07a1611c5c3..5cc436c335edd 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -46,7 +46,7 @@ impl PidChecking { let actual_pid = process::id(); if require_pid != actual_pid { unsafe { - libc::raise(libc::SIGTRAP); + libc::raise(libc::SIGUSR1); } } } @@ -146,5 +146,5 @@ fn main() { let status = run(&|| panic!("allocating to display... {}", DisplayWithHeap)); dbg!(status); - assert_eq!(status.signal(), Some(libc::SIGTRAP)); + assert_eq!(status.signal(), Some(libc::SIGUSR1)); } From 6369637a192bbd0a2fbf8084345ddb7c099aa460 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 13 May 2021 18:41:18 +0100 Subject: [PATCH 13/14] 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(-) 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 d7915d81ebdec..157debf2d257b 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 5cc436c335edd..f178baefcf621 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() { From 88ccaa77f17a74ec8597efa5c86f0f789028d1b4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 14 May 2021 11:38:25 +0100 Subject: [PATCH 14/14] panic abort after fork test: Disable on android And link to the issue. Signed-off-by: Ian Jackson --- src/test/ui/process/process-panic-after-fork.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index f178baefcf621..1ccf6bb051c20 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -5,6 +5,7 @@ // ignore-sgx no libc // ignore-emscripten no processes // ignore-sgx no processes +// ignore-android: FIXME(#85261) #![feature(bench_black_box)] #![feature(rustc_private)]