Who is responsible for preventing reentrancy issues through the allocator? #506
Comments
|
Actually IIUC 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 |
|
swc-project/swc#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:
No, the alloc error handler in std will call
Those are very different; they may only do async-signal-safe things and they are generally accepted as a very non-standard execution environment.
So we should forbid panics but also forbidding panics is untenable...? |
|
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 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.) |
|
Making the global allocator be the one for ensuring reentrancy seems to be the most reasonable option. Yes it's difficult, but it seems natural that the "code in charge of global dynamic memory allocation" should shoulder more responsibility, rather than having all other unsafe code/data structures have to worry about being reentered whenever they allocate memory.
Yes, since the allocator currently isn't allowed to panic. Otherwise this code: is unsound if use std::alloc::{GlobalAlloc, Layout, System};
use std::cell::Cell;
use std::mem::ManuallyDrop;
use std::rc::{Rc, Weak};
use std::sync::atomic::{AtomicBool, Ordering};
type RcBox = Rc<Box<i32>>;
type WeakBox = Weak<Box<i32>>;
struct Demonic;
thread_local! {
static STRONG: Cell<Option<RcBox>> = const { Cell::new(None) };
static WEAK: Cell<WeakBox> = const { Cell::new(Weak::new()) };
}
static SIDE_CHANNEL: AtomicBool = AtomicBool::new(false);
fn dupe_rc() {
// `ManuallyDrop` used to avoid an overflow check in debug mode
STRONG.set(ManuallyDrop::new(WEAK.take()).upgrade());
}
unsafe impl GlobalAlloc for Demonic {
unsafe fn alloc(&self, l: Layout) -> *mut u8 {
if SIDE_CHANNEL.load(Ordering::Relaxed) {
SIDE_CHANNEL.store(false, Ordering::Relaxed);
dupe_rc();
}
System.alloc(l)
}
unsafe fn dealloc(&self, c: *mut u8, l: Layout) {
System.dealloc(c, l);
}
}
#[global_allocator]
static D: Demonic = Demonic;
fn main() {
let mut oblivious = RcBox::default();
WEAK.set(Rc::downgrade(&oblivious));
SIDE_CHANNEL.store(true, Ordering::Relaxed);
Rc::make_mut(&mut oblivious);
drop(oblivious);
// double free. whose fault is it?
STRONG.set(None);
}miri backtraceEDIT: made the example less pathological/more realistic |
This isn't fully clear in the current docs; they say that
but this may refer either to any attempt to initiate a panic within the scope of allocation, or only to an attempt to unwind from (out of) an allocation function.
Good proof of concept. It looks like But this does clearly illustrate the difficulty of accounting for allocation being a potential reentrancy point and I think put it on a similar footing as the Additionally, I think this illustrates that if the panicking restriction isn't total and only raises library UB at the point of an unwinding out of an alloc function, then we need to have a flag in the panic count that prevents calling into the panic hook for a panic in the allocator. It's probably also a good idea to put a helpful |
|
That is a neat example. :) However, your code link points at a That's a call to Cc @rust-lang/wg-allocators for some more input on the intended soundness contract here.
The same example could be constructed using custom local allocators, right? We can't automatically add a reentrancy guard to those, I think... |
Yeah... also, if we take away their ability to panic, we should give them some other way to halt execution. And even then it's an enormous footgun as rustc will insert a bunch of panics automatically... so probably what we should rather do is have a function like |
Yeah, sorry for the confusion. I didn't know which part to point out, so I highlighted the part where the |
There is no way to implement such a function for no_std programs. It did need a thread local variable, which requires std. |
The cost of toggling a thread-local variable, while small, is not ideal for something as hot as an allocator. I can imagine schemes where it would instead just be checking a global variable, but that would still be a measurable expense. I wish Rust had an effect system (although I like @nikomatsakis' "function flavor" term better). In this case, allocators could use a "no panic" flavor to ensure they never called into any code that could panic. There's been a drumbeat of requests for a "no panic" feature over the years, and this would be one more use case. Admittedly, a flavor system would not be a panacea; we would still be left with the problem that writing no-panic code is (currently) extremely unergonomic. But that's solvable. Without a flavor system, though, we can't even get started on solving it. |
Calling the global allocator can in turn call arbitrary user-defined code
-Zoom=panic(unstable) plus a custom panic hook#[alloc_error_handler](unstable, requires no_std)#[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_unstablesymbol makes that impossible though? @bjorn3 might know.Similar to #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?
The text was updated successfully, but these errors were encountered: