about summary refs log tree commit diff
path: root/library/std/src/sys
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 /library/std/src/sys
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 'library/std/src/sys')
-rw-r--r--library/std/src/sys/unix/process/process_unix.rs1
-rw-r--r--library/std/src/sys/unix/process/process_unix/tests.rs27
2 files changed, 28 insertions, 0 deletions
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()) };
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..157debf2d25 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,10 @@
+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.
@@ -28,3 +35,23 @@ fn exitstatus_display_tests() {
         t(0x000ff, "unrecognised wait status: 255 0xff");
     }
 }
+
+#[test]
+#[cfg_attr(target_os = "emscripten", ignore)]
+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 || signal == libc::SIGTRAP);
+}