Skip to content

Commit

Permalink
Auto merge of #102460 - flba-eb:fix_85261_prevent_alloc_after_fork, r…
Browse files Browse the repository at this point in the history
…=thomcc

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.

Fixes #85261 (not only on Android systems, but also on Linux/QNX with TLS disabled, see issue for more details)

Main drawbacks of this fix:
* Panic messages can incorrectly omit `core::panic::PanicInfo` struct in case several panics (of multiple threads) occur at the same time. The handler cannot distinguish between multiple panics in different threads or recursive ones in the same thread, but the message will contain a hint about the uncertainty.
* `panic_count::increase()` will be a bit slower as it has an additional `if`, but this should be irrelevant as it is only called in case of a panic.
  • Loading branch information
bors committed Oct 12, 2022
2 parents e6ce562 + 4c5d6bb commit 50f6d33
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 5 deletions.
27 changes: 23 additions & 4 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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() {
Expand Down
43 changes: 42 additions & 1 deletion src/test/ui/process/process-panic-after-fork.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
// ignore-sgx no libc
// ignore-emscripten no processes
// ignore-sgx no processes
// ignore-android: FIXME(#85261)
// ignore-fuchsia no fork

#![feature(rustc_private)]
Expand Down Expand Up @@ -79,7 +78,49 @@ unsafe impl<A:GlobalAlloc> GlobalAlloc for PidChecking<A> {
fn expect_aborted(status: ExitStatus) {
dbg!(status);
let signal = status.signal().expect("expected child process to die of signal");

#[cfg(not(target_os = "android"))]
assert!(signal == libc::SIGABRT || signal == libc::SIGILL || signal == libc::SIGTRAP);

#[cfg(target_os = "android")]
{
// Android signals an abort() call with SIGSEGV at address 0xdeadbaad
// See e.g. https://groups.google.com/g/android-ndk/c/laW1CJc7Icc
assert!(signal == libc::SIGSEGV);

// Additional checks performed:
// 1. Find last tombstone (similar to coredump but in text format) from the
// same executable (path) as we are (must be because of usage of fork):
// This ensures that we look into the correct tombstone.
// 2. Cause of crash is a SIGSEGV with address 0xdeadbaad.
// 3. libc::abort call is in one of top two functions on callstack.
// The last two steps distinguish between a normal SIGSEGV and one caused
// by libc::abort.

let this_exe = std::env::current_exe().unwrap().into_os_string().into_string().unwrap();
let exe_string = format!(">>> {this_exe} <<<");
let tombstone = (0..100)
.map(|n| format!("/data/tombstones/tombstone_{n:02}"))
.filter(|f| std::path::Path::new(&f).exists())
.map(|f| std::fs::read_to_string(&f).expect("Cannot read tombstone file"))
.filter(|f| f.contains(&exe_string))
.last()
.expect("no tombstone found");

println!("Content of tombstone:\n{tombstone}");

assert!(
tombstone.contains("signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr deadbaad")
);
let abort_on_top = tombstone
.lines()
.skip_while(|l| !l.contains("backtrace:"))
.skip(1)
.take_while(|l| l.starts_with(" #"))
.take(2)
.any(|f| f.contains("/system/lib/libc.so (abort"));
assert!(abort_on_top);
}
}

fn main() {
Expand Down

0 comments on commit 50f6d33

Please sign in to comment.