handle_alloc_error + UnsafeCell = ???

[workingjubilee]

The question I have is the combination of the following code, using the swc_atoms crate, with the alloc_error_hook feature, as discussed by @LegionMammal978 in swc-project/swc#8362 (comment)

// cargo +nightly run --release --target i686-unknown-linux-gnu
#![forbid(unsafe_code)]
#![feature(alloc_error_hook)]
use std::alloc;
use swc_atoms::AtomStoreCell;

fn main() {
    thread_local! {
        static STORE: AtomStoreCell = AtomStoreCell::default();
    }
    alloc::set_alloc_error_hook(|_| {
        STORE.with(|store| {
            println!("start of inner call");
            store.atom("example");
            println!("end of inner call");
        });
    });
    let vec = vec![0; isize::MAX as usize];
    let s = String::from_utf8(vec).unwrap();
    STORE.with(|store| {
        println!("start of outer call");
        store.atom(&s);
        println!("end of outer call");
    });
}

Essentially, because the data structure first obtains &mut to itself via UnsafeCell, and then begins meddling with allocations, then if an allocation error handler is triggered during that moment and also obtains that &mut, we get an aliasing &mut. This makes the data structure's interface effectively unsound in the presence of the alloc error handler, and probably also panic handlers?

So the question is: is this data structure indeed unsound? But also, even if we decree AtomStoreCell unsound, this raises a question about these kinds of hooks and how we should teach making safe data structures in their presence, as it implies even !Sync structures must worry about, effectively, reentrancy.

[RalfJung]

Sounds like an instance of "reentrancy through the allocator".

What is the data structure doing that this is a problem here but not e.g. with RefCell? Using RefCell in thread-local storage I can also have an &mut to some data and then call the allocator and then try to access the same RefCell again, but of course that will work fine since RefCell properly guards against reentrancy. Why does AtomStoreCell not do that? Would be good to get a summary of that so that we don't have to reverse-engineer this from the AtomStoreCell sources. :)

[digama0]

Using RefCell in thread-local storage I can also have an &mut to some data and then call the allocator and then try to access the same RefCell again, but of course that will work fine since RefCell properly guards against reentrancy. Why does AtomStoreCell not do that?

Because performance, naturally. I think they were previously using a RefCell, but they deliberately skip the check:

/// A fast internally mutable cell for [AtomStore].
#[derive(Default)]
pub struct AtomStoreCell(UnsafeCell<AtomStore>);

impl AtomStoreCell {
    #[inline]
    pub fn atom<'a>(&self, s: impl Into<Cow<'a, str>>) -> Atom {
        // evaluate the into before borrowing (see #8362)
        let s: Cow<'a, str> = s.into();
        // SAFETY: We can skip the borrow check of RefCell because
        // this API enforces a safe contract. It is slightly faster
        // to use an UnsafeCell. Note the borrow here is short lived
        // only to this block.
        unsafe { (*self.0.get()).atom(s) }
    }
}

Whether "this API enforces a safe contract" is what is being litigated in swc-project/swc#8362.

I believe the details of AtomStore don't matter here, except that it has a pub fn atom<'a>(&mut self, s: impl Into<Cow<'a, str>>) -> Atom function which indirectly calls the allocator via functions like Arc::new.

[RalfJung]

So they basically assume there is no reentrancy, but that assumption is violated through the allocator.
(I don't actually see the allocation causing the reentrancy here, I guess it is inside the atomic function?)

This sounds like a libs-api and lang question to me: do we expect all unsafe code to support reentrancy through the allocator, allocation-error hook, and panic hook -- or does the code running in those contexts have to follow "special rules"? Given that allocation-error hook and panic hook do not have any such requirement documented, I think currently the position is that unsafe code must be able to deal with such reentrancy and therefore AtomStoreCell is unsound.

[RalfJung]

I opened issues to track the reentrancy question, as it's not really specifically about this code:

• Better documentation needed: reentrancy through the panic hook #505
• Who is responsible for preventing reentrancy issues through the allocator? #506

This one here can be closed then, I think. Thanks for bringing this up!