From 8e70c82f572be26a9d838e52f451b270160ffdba Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 29 Sep 2022 11:04:25 +0200 Subject: Prevent UB in child process after calling libc::fork After calling libc::fork, the child process tried to access a TLS variable when processing a panic. This caused a memory allocation which is UB in the child. To prevent this from happening, the panic handler will not access the TLS variable in case `panic::always_abort` was called before. --- library/std/src/panicking.rs | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 4b07b393a2f..cd4d41fe123 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -308,6 +308,14 @@ pub mod panic_count { // 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. + // panic::always_abort() is usually called to prevent memory allocations done by + // the panic handling in the child created by `libc::fork`. + // Memory allocations performed in a child created with `libc::fork` are undefined + // behavior in most operating systems. + // Accessing LOCAL_PANIC_COUNT in a child created by `libc::fork` would lead to a memory + // allocation. Only GLOBAL_PANIC_COUNT can be accessed in this situation. This is + // sufficient because a child process will always have exactly one thread only. + // See also #85261 for details. // // 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, @@ -318,15 +326,26 @@ pub mod panic_count { // panicking thread consumes at least 2 bytes of address space. static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0); + // Return the state of the ALWAYS_ABORT_FLAG and number of panics. + // + // If ALWAYS_ABORT_FLAG is not set, the number is determined on a per-thread + // base (stored in LOCAL_PANIC_COUNT), i.e. it is the amount of recursive calls + // of the calling thread. + // If ALWAYS_ABORT_FLAG is set, the number equals the *global* number of panic + // calls. See above why LOCAL_PANIC_COUNT is not used. pub fn increase() -> (bool, usize) { - ( - GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed) & ALWAYS_ABORT_FLAG != 0, + let global_count = GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed); + let must_abort = global_count & ALWAYS_ABORT_FLAG != 0; + let panics = if must_abort { + global_count & !ALWAYS_ABORT_FLAG + } else { LOCAL_PANIC_COUNT.with(|c| { let next = c.get() + 1; c.set(next); next - }), - ) + }) + }; + (must_abort, panics) } pub fn decrease() { -- cgit 1.4.1-3-g733a5 From 9a97cc8ca5f863cacf39c9aaa4ca6ad872bc8172 Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 6 Oct 2022 08:52:36 +0200 Subject: Fix whitespace --- library/std/src/panicking.rs | 4 ++-- src/test/ui/process/process-panic-after-fork.rs | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index cd4d41fe123..d4976a469cc 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -309,10 +309,10 @@ pub mod panic_count { // records whether panic::always_abort() has been called. This can only be // set, never cleared. // panic::always_abort() is usually called to prevent memory allocations done by - // the panic handling in the child created by `libc::fork`. + // the panic handling in the child created by `libc::fork`. // Memory allocations performed in a child created with `libc::fork` are undefined // behavior in most operating systems. - // Accessing LOCAL_PANIC_COUNT in a child created by `libc::fork` would lead to a memory + // Accessing LOCAL_PANIC_COUNT in a child created by `libc::fork` would lead to a memory // allocation. Only GLOBAL_PANIC_COUNT can be accessed in this situation. This is // sufficient because a child process will always have exactly one thread only. // See also #85261 for details. diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index 43c23b49737..b08aa895dec 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -99,7 +99,9 @@ fn expect_aborted(status: ExitStatus) { // If the next assert fails sporadically we might have an issue with parallel crashing apps assert!(tombstone .contains(&std::env::current_exe().unwrap().into_os_string().into_string().unwrap())); - assert!(tombstone.contains("signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr deadbaad")); + assert!(tombstone.contains( + "signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr deadbaad" + )); } } -- cgit 1.4.1-3-g733a5