Who is responsible for preventing reentrancy issues through the allocator?

[RalfJung]

Calling the global allocator can in turn call arbitrary user-defined code

• if there is a custom allocator (needs unsafe)
• if allocation fails, and there is one of
  • a custom alloc hook (unstable)
  • -Zoom=panic (unstable) plus a custom panic hook
  • a custom #[alloc_error_handler] (unstable, requires no_std)
  • a no_std library using alloc's default error hook (which panics) with a #[panic_handler] (may be possible on stable?)

It may just barely be the case that this cannot be triggered on stable today, depending on whether there is any way to actually run the liballoc default error hook on stable. I think the __rust_no_alloc_shim_is_unstable symbol makes that impossible though? @bjorn3 might know.

Similar to https://github.com/rust-lang/unsafe-code-guidelines/issues/505, this can lead to reentrancy. That has caused real soundness questions: is it the responsibility of the allocator and the alloc error hook to never call into data structures except if they are declared reentrant, or is it the responsibility of every data structure to be resilient against reentrancy through the allocator? If they anyway have to be able to deal with reentrant panic hooks (due to #505), then is the extra possibility of reentrancy via the allocator even relevant?

[CAD97]

Actually IIUC handle_alloc_error with -Zoom=abort still calls the panic hook, it just does so with can_unwind=false so no unwinding occurs.

A more niche but more obviously suspect case of potential surprise single thread reentrancy is signal handlers. At least there the usual model is that they're a separate logical thread of execution even if physically they run within an existing thread.

I think "global allocator implementation shouldn't perform allocation" is a common rule, which would include forbidding panics. But I think any scenario which says panicking at all is unsound without even being able to cover it with catch_unwind is untenable.

[RalfJung]

https://github.com/swc-project/swc/issues/8362 is likely still an issue in the latest version of that crate. If there is a custom allocator, and that allocator is allowed to panic, then one can cause an aliasing violation in entirely safe code:

• call AtomStoreCell::atomic
• that will create an &mut and pass it to AtomStore::atom (meaning it is protected and invalidation is insta-UB)
• that will call some things in the hstr crate which end up doing an allocation
• that will call our allocator, which panics
• that will call our panic hook, which calls AtomStoreCell::atomic a second time which creates an aliasing &mut which is immediate UB due to the protector created the first time around

Actually IIUC handle_alloc_error with -Zoom=abort still calls the panic hook, it just does so with can_unwind=false so no unwinding occurs.

No, the alloc error handler in std will call rtprintpanic! which does not call the panic hook.

signal handlers

Those are very different; they may only do async-signal-safe things and they are generally accepted as a very non-standard execution environment.

I think "global allocator implementation shouldn't perform allocation" is a common rule, which would include forbidding panics. But I think any scenario which says panicking at all is unsound without even being able to cover it with catch_unwind is untenable.

So we should forbid panics but also forbidding panics is untenable...?

[CAD97]

To restate hopefully a bit clearer:

I believe "global allocators don't reentrantly call the global allocator" is a common property that people expect to be upheld. However, I don't think that treating this as a soundness property that must be upheld by the developer is a viable option (c.f. allocation size fitting isize). Since arbitrary panic hooks may of course include allocation, such a requirement means writing entirely panic-free code, elevating accidental panics from a likely fatal error to unsoundness.

There's two sides to the coin here: reentrancy in code because the allocator reentered it, and reentrancy of the allocator itself. My observation is more about the latter, not the former. Making code no-global-alloc is a reasonable ask (the allocator won't enter your data structure that uses the allocator), but no-panic isn't really, not without better compiler assistance anyway. So perhaps this ends up more about #505 (reentrancy via panic hook) than this issue (reentrancy via alloc hook).

However, it's possible to imagine the Rust runtime enforcing alloc reentrancy freedom in the global allocator dynamically, by manipulating the alloc and/or panic hook. (But if safe APIs must tolerate reentrancy from the panic hook anyway, preventing alloc reentrancy doesn't seem actually beneficial.)