Allocation after libc::fork on Android

[ijackson]

I am trying to fix it so that Rust's stdlib prevents unwinding, or allocating, in the child, after a fork on Unix (including in Command). That is #81858. (Allocation after fork of a multithreaded program is UB in several libcs.)

I added a new test case, https://github.com/rust-lang/rust/blob/8220f2f2127b9aec972163ded97be7d8cff6b9a8/src/test/ui/process/process-panic-after-fork.rs https://github.com/rust-lang/rust/blob/6369637a192bbd0a2fbf8084345ddb7c099aa460/src/test/ui/process/process-panic-after-fork.rs Unfortunately this test fails, but just on Android: #81858 (comment)

I have few good theories as to why. I wrote some speculations: #81858 (comment)

I think this probably needs attention from an Android expert to try to repro and fix this issue. I suspect it's a problem with the library rather than the tests. The worst case is that it might be a general UB bug in Android Rust programs using libc::fork or Command.

I'm filing this issue here to try to ask for help again, since writing in #81858 doesn't seem like a particularly good way of getting the attention of Android folks.

If we can't get a resolution, reluctantly, I guess I will disable that test on Android so that my MR can go through. The current situation is quite a hazard (see eg #79740 "panic! in Command child forked from non-main thread results in exit status 0")

Technical discussion

I will try to explain what the test does and what the symptoms seem to mean:

The test file has a custom global allocator, whose purpose is to spot allocations in the child after fork. That global allocator has an atomic variable which is supposed to contain either zero (initially, meaning it's not engaged yet) or the process's pid. Whenever an allocator method is called, we read the atomic and, if it is not zero, we check it against process::id(). If it doesn't match we libc::raise(libc::SIGTRAP SIGUSR1).

The test enters main, and engages the stunt allocator, recording the program's pid. Each call to expect_aborted (which is called from run and therefore from one) produces output from dbg!(status). We see only one of these, so this must be the first test, one(&|| panic!()).

The test uses libc::fork to fork. In the child, it calls panic::always_abort() (my new function to disable panic unwinding). It then panics (using the provided closure). This ought to result in the program dying with SIGABRT (or maybe SIGILL or SIGTRAP).

The parent collects the child's exit status. For the first test case, we run expect_aborted. This extracts the signal number from it and checks that it is as expected. On other systems this works.

In the failing test, this test fails. The assertion on signal fails. Meaning, the child did die of a signal but the signal number wasn't the one expected. The previous debug print shows that the raw wait status (confusingly described by Rust stdlib as an "exit status") is 510. Usually, a bare number like that in a wait status is a signal number, and indeed that seems to be the case here since status.signal() is Some(...). On Linux (and most Unices), 5 is SIGTRAP and 10 is SIGUSR1.

Ie, it seems that the child tried to allocate memory, despite my efforts to make sure that panicking does not involve allocation. Weirdly, a more-portable test case which uses Command and does not insist on specific signal numbers passes.

It's definitely my stunt allocator which is tripping here, because when I changed it to use SIGUSR instead of SIGTRAP, the failing test case signal number changed too.

[ijackson]

@rustbot modify labels +O-android +T-libs-impl

[ijackson]

@hyd-dev provided an explanation here #81858 (comment)

I still think there is a bug here but it is not Android-specific. I have a WIP branch to try to address it, so I think it's probably best if I close this issue and submit that as an MR when it's ready.

[ijackson]

My first guesses were right. In #81858 (comment) where I changed the stunt allocator to use SIGUSR1, we see this:

2021-05-14T04:09:46.9170083Z [/checkout/src/test/ui/process/process-panic-after-fork.rs:79] status = ExitStatus(
2021-05-14T04:09:46.9171632Z     ExitStatus(
2021-05-14T04:09:46.9172360Z         10,
2021-05-14T04:09:46.9172996Z     ),
2021-05-14T04:09:46.9173607Z )

I found a memory allocation with a patched Miri:

note: tracking was triggered
  --> /home/hyd-dev/Code/rust-lang/rust/library/alloc/src/alloc.rs:86:14
   |
86 |     unsafe { __rust_alloc(layout.size(), layout.align()) }
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ rust memory allocated
   |
   = note: inside `std::alloc::alloc` at /home/hyd-dev/Code/rust-lang/rust/library/alloc/src/alloc.rs:86:14
   = note: inside `std::alloc::Global::alloc_impl` at /home/hyd-dev/Code/rust-lang/rust/library/alloc/src/alloc.rs:166:73
   = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /home/hyd-dev/Code/rust-lang/rust/library/alloc/src/alloc.rs:226:9
   = note: inside `alloc::alloc::exchange_malloc` at /home/hyd-dev/Code/rust-lang/rust/library/alloc/src/alloc.rs:316:11
   = note: inside `std::thread::__OsLocalKeyInner::<std::cell::Cell<usize>>::try_initialize` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/thread/local.rs:703:42
   = note: inside `std::thread::__OsLocalKeyInner::<std::cell::Cell<usize>>::get` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/thread/local.rs:685:22
   = note: inside `std::rt::panic_count::LOCAL_PANIC_COUNT::__getit` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/thread/local.rs:287:26
   = note: inside `std::thread::LocalKey::<std::cell::Cell<usize>>::try_with::<[closure@std::rt::panic_count::increase::{closure#0}], usize>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/thread/local.rs:375:32
   = note: inside `std::thread::LocalKey::<std::cell::Cell<usize>>::with::<[closure@std::rt::panic_count::increase::{closure#0}], usize>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/thread/local.rs:352:9
   = note: inside `std::rt::panic_count::increase` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:263:13
   = note: inside `std::panicking::rust_panic_with_hook` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:588:32
   = note: inside closure at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:542:9
   = note: inside `std::sys_common::backtrace::__rust_end_short_backtrace::<[closure@std::rt::begin_panic<&str>::{closure#0}], !>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/sys_common/backtrace.rs:141:18
   = note: inside `std::rt::begin_panic::<&str>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:541:12
note: inside closure at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panic.rs:28:9
  --> src/main.rs:11:13
   |
11 |     run(&|| panic!());
   |             ^^^^^^^^
note: inside `run` at src/main.rs:7:5
  --> src/main.rs:7:5
   |
7  |     do_panic();
   |     ^^^^^^^^^^
note: inside `main` at src/main.rs:11:5
  --> src/main.rs:11:5
   |
11 |     run(&|| panic!());
   |     ^^^^^^^^^^^^^^^^^
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/hyd-dev/Code/rust-lang/rust/library/core/src/ops/function.rs:227:5
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/sys_common/backtrace.rs:125:18
   = note: inside closure at /home/hyd-dev/Code/rust-lang/rust/library/std/src/rt.rs:49:18
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/hyd-dev/Code/rust-lang/rust/library/core/src/ops/function.rs:259:13
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:401:40
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panicking.rs:365:19
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/panic.rs:433:14
   = note: inside `std::rt::lang_start_internal` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/rt.rs:34:21
   = note: inside `std::rt::lang_start::<()>` at /home/hyd-dev/Code/rust-lang/rust/library/std/src/rt.rs:48:5
   = note: this note originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)

With this code:

#![feature(panic_always_abort)]

use std::panic;

fn run(do_panic: &dyn Fn()) {
    panic::always_abort();
    do_panic();
}

fn main() {
    run(&|| panic!());
}

[ijackson]

Apparently, the thread locals aren't working as expected. I think the child after fork is being treated as a new thread, and then the thread local storage has to be re-created. I think the surviving thread in the fork should inherit all the thread-locals from the forking thread in the parent.

Apparently, the thread locals aren't working as expected. I think the child after fork is being treated as a new thread, and then the thread local storage has to be re-created. I think the surviving thread in the fork should inherit all the thread-locals from the forking thread in the parent.

But I just tested the code after fork(), so there's no "new thread" or "parent" here...

[ijackson]

Oh! Yes. You are right. Evidently the first use of this thread-local allocates. Presumably that would happen on a new thread as well as on the initial thread.

Having panic rely on thread-locals, and thread-locals rely on the allocator, seems unfortunate. Having panic allocate is not good, especially when actually it's just trying to abort precisely to avoid this.

Since ALWAYS_ABORT_FLAG is stored in GLOBAL_PANIC_COUNT which is not a thread-local, maybe the program could abort before using any thread-local.