From 8e70c82f572be26a9d838e52f451b270160ffdba Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 29 Sep 2022 11:04:25 +0200 Subject: [PATCH 1/6] 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(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 4b07b393a2f5a..cd4d41fe12377 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() { From 8b821ccc51fef460a97b5dd299421c91730537f5 Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 29 Sep 2022 11:18:23 +0200 Subject: [PATCH 2/6] Enable test on Android --- src/test/ui/process/process-panic-after-fork.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index d0a938c03e803..9b0f9f82f8f69 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -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)] From 56503460c88844a61fbf47cc6702e206355b0a2b Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 6 Oct 2022 08:43:27 +0200 Subject: [PATCH 3/6] Fix test for Android --- .../ui/process/process-panic-after-fork.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index 9b0f9f82f8f69..43c23b49737d0 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -78,7 +78,29 @@ unsafe impl GlobalAlloc for PidChecking { 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); + // Check if the crash occured at addres deadbaad to ensure it is not some undefined + // behavior but actually an abort + let tombstone = (0..100) + .map(|n| format!("/data/tombstones/tombstone_{n:02}")) + .filter(|f| std::path::Path::new(&f).exists()) + .last() + .expect("no tombstone found"); + let tombstone = + std::fs::read_to_string(&tombstone).expect("Cannot read tombstone file"); + // 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")); + } } fn main() { From 9a97cc8ca5f863cacf39c9aaa4ca6ad872bc8172 Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 6 Oct 2022 08:52:36 +0200 Subject: [PATCH 4/6] 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(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index cd4d41fe12377..d4976a469cc15 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 43c23b49737d0..b08aa895dec5a 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" + )); } } From 53caa9fafbf199c8dd1b312203045a618e1c38dc Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Thu, 6 Oct 2022 15:11:21 +0200 Subject: [PATCH 5/6] Ensure crash is caused by libc::abort --- .../ui/process/process-panic-after-fork.rs | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index b08aa895dec5a..ed4c2b5bd3924 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -87,21 +87,39 @@ fn expect_aborted(status: ExitStatus) { // 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); - // Check if the crash occured at addres deadbaad to ensure it is not some undefined - // behavior but actually an abort - let tombstone = (0..100) + + // Additional checks performed: + // 1. Crash is from same executable (path) as we are (must be because 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 tombstone_name = (0..100) .map(|n| format!("/data/tombstones/tombstone_{n:02}")) .filter(|f| std::path::Path::new(&f).exists()) .last() .expect("no tombstone found"); + let tombstone = - std::fs::read_to_string(&tombstone).expect("Cannot read tombstone file"); + std::fs::read_to_string(&tombstone_name).expect("Cannot read tombstone file"); + println!("Content of {tombstone_name}:\n{tombstone}"); + // 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" )); + 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); } } From 4c5d6bb490af0b046c4e445ae90edc0c38623591 Mon Sep 17 00:00:00 2001 From: Florian Bartels Date: Fri, 7 Oct 2022 10:22:01 +0200 Subject: [PATCH 6/6] Ensure the correct tombstone file is opened --- .../ui/process/process-panic-after-fork.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index ed4c2b5bd3924..6d4d24922253d 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -89,29 +89,29 @@ fn expect_aborted(status: ExitStatus) { assert!(signal == libc::SIGSEGV); // Additional checks performed: - // 1. Crash is from same executable (path) as we are (must be because of fork): + // 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 tombstone_name = (0..100) + 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"); - let tombstone = - std::fs::read_to_string(&tombstone_name).expect("Cannot read tombstone file"); - println!("Content of {tombstone_name}:\n{tombstone}"); + println!("Content of tombstone:\n{tombstone}"); - // 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") + ); let abort_on_top = tombstone .lines() .skip_while(|l| !l.contains("backtrace:"))