about summary refs log tree commit diff
path: root/src
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2021-05-15 22:27:09 +0000
committerbors <bors@rust-lang.org>2021-05-15 22:27:09 +0000
commitd565c7488749fd0e998d6be21efeb20354e4696d (patch)
tree0737eae74f62313ce2a265bdf314ba1d8f9d8645 /src
parent8cf990c9b5c59f25c806fad9f4466f9d6509bbea (diff)
parent88ccaa77f17a74ec8597efa5c86f0f789028d1b4 (diff)
downloadrust-d565c7488749fd0e998d6be21efeb20354e4696d.tar.gz
rust-d565c7488749fd0e998d6be21efeb20354e4696d.zip
Auto merge of #81858 - ijackson:fork-no-unwind, r=m-ou-se
Do not allocate or unwind after fork

### Objective scenarios

 * Make (simple) panics safe in `Command::pre_exec_hook`, including most `panic!` calls, `Option::unwrap`, and array bounds check failures.
 * Make it possible to `libc::fork` and then safely panic in the child (needed for the above, but this requirement means exposing the new raw hook API which the `Command` implementation needs).
 * In singlethreaded programs, where panic in `pre_exec_hook` is already memory-safe, prevent the double-unwinding malfunction #79740.

I think we want to make panic after fork safe even though the post-fork child environment is only experienced by users of `unsafe`, beause the subset of Rust in which any panic is UB is really far too hazardous and unnatural.

#### Approach

 * Provide a way for a program to, at runtime, switch to having panics abort.  This makes it possible to panic without making *any* heap allocations, which is needed because on some platforms malloc is UB in a child forked from a multithreaded program (see https://github.com/rust-lang/rust/pull/80263#issuecomment-774272370, and maybe also the SuS [spec](https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html)).
 * Make that change in the child spawned by `Command`.
 * Document the rules comprehensively enough that a programmer has a fighting chance of writing correct code.
 * Test that this all works as expected (and in particular, that there aren't any heap allocations we missed)

Fixes #79740

#### Rejected (or previously attempted) approaches

 * Change the panic machinery to be able to unwind without allocating, at least when the payload and message are both `'static`.  This seems like it would be even more subtle.  Also that is a potentially-hot path which I don't want to mess with.
 * Change the existing panic hook mechanism to not convert the message to a `String` before calling the hook.  This would be a surprising change for existing code and would not be detected by the type system.
 * Provide a `raw_panic_hook` function to intercept panics in a way that doesn't allocate.  (That was an earlier version of this MR.)

### History

This MR could be considered a v2 of #80263.  Thanks to everyone who commented there.  In particular, thanks to `@m-ou-se,` `@Mark-Simulacrum` and `@hyd-dev.`  (Tagging you since I think you might be interested in this new MR.)  Compared to #80263, this MR has very substantial changes and additions.

Additionally, I have recently (2021-04-20) completely revised this series following very helpful comments from `@m-ou-se.`

r? `@m-ou-se`
Diffstat (limited to 'src')
-rw-r--r--src/test/ui/panics/abort-on-panic.rs85
-rw-r--r--src/test/ui/process/process-panic-after-fork.rs151
2 files changed, 213 insertions, 23 deletions
diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs
index f34cf6a9cbf..2aea607e954 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() {
@@ -23,41 +26,77 @@ 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 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();
+}
+
 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 test_always_abort() {
+    panic::always_abort();
+    let _ = panic::catch_unwind(|| { panic!(); });
+    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<String> = 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() }
         }
+        bomb_out_but_not_abort("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();
+        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 {
+        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..1ccf6bb051c
--- /dev/null
+++ b/src/test/ui/process/process-panic-after-fork.rs
@@ -0,0 +1,151 @@
+// run-pass
+// no-prefer-dynamic
+// ignore-wasm32-bare no libc
+// ignore-windows
+// ignore-sgx no libc
+// ignore-emscripten no processes
+// ignore-sgx no processes
+// ignore-android: FIXME(#85261)
+
+#![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<A> {
+    parent: A,
+    require_pid: AtomicU32,
+}
+
+#[global_allocator]
+static ALLOCATOR: PidChecking<std::alloc::System> = PidChecking {
+    parent: std::alloc::System,
+    require_pid: AtomicU32::new(0),
+};
+
+impl<A> PidChecking<A> {
+    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::SIGUSR1);
+                }
+            }
+        }
+    }
+}
+
+unsafe impl<A:GlobalAlloc> GlobalAlloc for PidChecking<A> {
+    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 || signal == libc::SIGTRAP);
+}
+
+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::<DisplayWithHeap>.unwrap(); });
+    one(&|| { None::<DisplayWithHeap>.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::SIGUSR1));
+}