-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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: https:/rust-lang/rust/blob/ef4e8259b5016d85e261587b605028b2ff06c13d/library/alloc/src/rc.rs#L1877 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 backtrace
EDIT: made the example less pathological/more realistic |
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_unstable
symbol 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: