AtomStoreCell can create aliasing mutable references

[LegionMammal978]

Describe the bug

The swc_atoms::AtomStoreCell::atom() function creates a &mut reference from *self.0.get(). The safety comment notes that "We can skip the borrow check of RefCell because this API enforces a safe contract." However, this is incorrect, since the function reenters user code to call .into(), and reenters it more times to allocate memory, and these can potentially call back into .atom(), creating a second &mut reference.

Input code

use std::borrow::Cow;
use swc_atoms::AtomStoreCell;

struct EvilStr<'a>(&'a AtomStoreCell);

impl<'a> From<EvilStr<'a>> for Cow<'a, str> {
    fn from(value: EvilStr<'a>) -> Self {
        value.0.atom(Cow::Borrowed("reentered!"));
        Cow::Borrowed("evil")
    }
}

fn main() {
    let cell = AtomStoreCell::default();
    cell.atom(EvilStr(&cell));
}

Config

No response

Playground link (or link to the minimal reproduction)

https://play.swc.rs/?version=1.3.100&code=H4sIAAAAAAAAA12Pz26DMAyH73kKj0ObVP2zc9ohtWh9AR5gysCRkAKZjCmHinefQ5nUzpco9pd8Px82augReq6t%2FY5EcbS2iOPx0R2rL8ex7a09y1FyJCwwhKNSPdNQMXzemlAyndYu16u1gxfMCNe0PyFN4UqxPT3hOfhIICq5bMVPOdwVSPkOvLD65sKA9tlgYJdDicEvZKqZ2r%2FvU0xdpPSXeQusdUaIHSNh%2FZYZyfL35B%2BFIsjMPJ3UpJT4W9d02iyWgAyVbAMfr9tZW6N3Q2C9%2FJ2gR44ls16lVlJPanP4BaeOXFBqAQAA&config=H4sIAAAAAAAAA1WPSw7DIAwF9zkF8rrbdtE79BAWdSIifrKJVBTl7iUE0maH3xsz8jooBbNoeKq1PMsQkYX4nEsi2Sf8lARIOxTNJia49XaWvRrRCtVoOxpIyBOluiX3hoMNQajjLXPGmzH%2FC3VwkUnkCu4o%2BsnSVTc0JbjwXmrZDkk50qF%2FwA%2FqsvNjMPLqm4kXGrYvhlQioBQBAAA%3D

SWC Info output

No response

Expected behavior

The function should detect that it has been reentered and panic.

Actual behavior

The function creates aliasing &mut references when reentered. Miri reports the following error:

error: Undefined Behavior: not granting access to tag <1849> because that would remove [Unique for <1822>] which is strongly protected because it is an argument of call 791
   --> /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.4/src/lib.rs:229:21
    |
229 |     pub fn atom<'a>(&mut self, s: impl Into<Cow<'a, str>>) -> Atom {
    |                     ^^^^^^^^^ not granting access to tag <1849> because that would remove [Unique for <1822>] which is strongly protected because it is an argument of call 791
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <1849> was created by a SharedReadWrite retag at offsets [0x0..0x28]
   --> src/main.rs:8:9
    |
8   |         value.0.atom(Cow::Borrowed("reentered!"));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <1822> is this argument
   --> src/main.rs:15:5
    |
15  |     cell.atom(EvilStr(&cell));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `swc_atoms::AtomStore::atom::<'_, std::borrow::Cow<'_, str>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.4/src/lib.rs:229:21: 229:30
    = note: inside `swc_atoms::AtomStoreCell::atom::<'_, std::borrow::Cow<'_, str>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.4/src/lib.rs:245:18: 245:41
note: inside `<impl std::convert::From<EvilStr<'_>> for std::borrow::Cow<'_, str>>::from`
   --> src/main.rs:8:9
    |
8   |         value.0.atom(Cow::Borrowed("reentered!"));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `<EvilStr<'_> as std::convert::Into<std::borrow::Cow<'_, str>>>::into` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/convert/mod.rs:757:9: 757:22
    = note: inside `swc_atoms::hstr::AtomStore::atom::<'_, EvilStr<'_>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:82:24: 82:35
    = note: inside `swc_atoms::AtomStore::atom::<'_, EvilStr<'_>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.4/src/lib.rs:230:14: 230:28
    = note: inside `swc_atoms::AtomStoreCell::atom::<'_, EvilStr<'_>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.4/src/lib.rs:245:18: 245:41
note: inside `main`
   --> src/main.rs:15:5
    |
15  |     cell.atom(EvilStr(&cell));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Version

swc_atoms 0.6.4

Additional context

My last issue, #8361, was automatically rejected for linking to the Rust Playground instead of the SWC Playground; it would be helpful if there were some upfront indication about the requirements for the provided link.

[kdy1]

cc @dsherret

[dsherret]

I mean, if someone wants to write code nobody would ever write then yeah. The Into should be evaluated outside the borrow to fix this.

[LegionMammal978]

I mean, if someone wants to write code nobody would ever write then yeah. The Into should be evaluated outside the borrow to fix this.

Another issue is that the function allocates memory (with at least a couple calls to Arc::new() I can find), and a user-defined allocator could gain access to the &AtomStoreCell and reenter .atom(). (And this could even make sense, e.g., if the allocator had a thread_local! AtomStoreCell, and just wanted to use .atom() within its implementation.) So just moving the .into() wouldn't be fully sufficient.

[dsherret]

I think what the code is doing is fine. Nobody is going to do that.

[workingjubilee]

Yes, unsafe code must remain sound in the presence of code "nobody would ever write". CVEs have been filed against std over less (and accepted! we fixed the problem!). You should eschew exposing functions that rely on safe traits not having sketchy-but-safe implementations.

An allocator, however, also is an unsafe impl with a set of unsafe fn, and so it is plausibly the allocator's responsibility to discharge the security against code "nobody would ever write".

[kdy1]

@LegionMammal978 Can you provide an example code? I'm not sure what you are talking about.

[kdy1]

Are you talking about #[global_allocator] defined by the user?

[LegionMammal978]

Are you talking about #[global_allocator] defined by the user?

Yes, I am referring to a user-defined #[global_allocator]. To give an explicit example where the AtomStoreCell is accessed by safe code, and the only unsafe code is to forward the allocation behavior from System:

#![forbid(unsafe_op_in_unsafe_fn)]
use std::{
    alloc::{GlobalAlloc, Layout, System},
    sync::atomic::{AtomicBool, Ordering},
};
use swc_atoms::AtomStoreCell;

struct EvilAlloc(AtomicBool);

#[global_allocator]
static ALLOC: EvilAlloc = EvilAlloc(AtomicBool::new(false));

// SAFETY: Wraps `System`'s methods, performing additional work.
unsafe impl GlobalAlloc for EvilAlloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        if self.0.load(Ordering::Relaxed) {
            additional_work();
        }
        // SAFETY: The layout is known to have a nonzero size.
        unsafe { System.alloc(layout) }
    }

    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        // SAFETY: The pointer and layout are known to denote allocated memory.
        unsafe { System.dealloc(ptr, layout) }
    }
}

fn additional_work() {
    thread_local! {
        static STORE: AtomStoreCell = AtomStoreCell::default();
    }
    ALLOC.0.store(false, Ordering::Relaxed);
    STORE.with(|store| {
        ALLOC.0.store(true, Ordering::Relaxed);
        store.atom("example");
    });
}

fn main() {
    ALLOC.0.store(true, Ordering::Relaxed);
    let _ = Box::new(0);
}

error: Undefined Behavior: not granting access to tag <3078> because that would remove [Unique for <2507>] which is strongly protected because it is an argument of call 1081
   --> /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.5/src/lib.rs:229:21
    |
229 |     pub fn atom<'a>(&mut self, s: impl Into<Cow<'a, str>>) -> Atom {
    |                     ^^^^^^^^^ not granting access to tag <3078> because that would remove [Unique for <2507>] which is strongly protected because it is an argument of call 1081
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3078> was created by a SharedReadWrite retag at offsets [0x8..0x30]
   --> src/main.rs:33:9
    |
33  |         store.atom("example");
    |         ^^^^^^^^^^^^^^^^^^^^^
help: <2507> is this argument
   --> src/main.rs:33:9
    |
33  |         store.atom("example");
    |         ^^^^^^^^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `swc_atoms::AtomStore::atom::<'_, std::borrow::Cow<'_, str>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.5/src/lib.rs:229:21: 229:30
    = note: inside `swc_atoms::AtomStoreCell::atom::<'_, &str>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.5/src/lib.rs:247:18: 247:41
note: inside closure
   --> src/main.rs:33:9
    |
33  |         store.atom("example");
    |         ^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::thread::LocalKey::<swc_atoms::AtomStoreCell>::try_with::<{closure@src/main.rs:31:16: 31:23}, ()>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:270:16: 270:31
    = note: inside `std::thread::LocalKey::<swc_atoms::AtomStoreCell>::with::<{closure@src/main.rs:31:16: 31:23}, ()>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:246:9: 246:25
note: inside `additional_work`
   --> src/main.rs:31:5
    |
31  | /     STORE.with(|store| {
32  | |         ALLOC.0.store(true, Ordering::Relaxed);
33  | |         store.atom("example");
34  | |     });
    | |______^
note: inside `<EvilAlloc as std::alloc::GlobalAlloc>::alloc`
   --> src/main.rs:14:13
    |
14  |             additional_work();
    |             ^^^^^^^^^^^^^^^^^
note: inside `_::__rust_alloc`
   --> src/main.rs:8:15
    |
7   | #[global_allocator]
    | ------------------- in this procedural macro expansion
8   | static ALLOC: EvilAlloc = EvilAlloc(AtomicBool::new(false));
    |               ^^^^^^^^^
    = note: inside `std::alloc::alloc` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:98:9: 98:52
    = note: inside `std::alloc::Global::alloc_impl` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:181:73: 181:86
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:241:9: 241:39
    = note: inside `alloc::raw_vec::RawVec::<u8>::allocate_in` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:184:45: 184:67
    = note: inside `alloc::raw_vec::RawVec::<u8>::with_capacity_in` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:130:9: 130:69
    = note: inside `std::vec::Vec::<u8>::with_capacity_in` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:670:20: 670:61
    = note: inside `<u8 as std::slice::hack::ConvertVec>::to_vec::<std::alloc::Global>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/slice.rs:162:25: 162:62
    = note: inside `std::slice::hack::to_vec::<u8, std::alloc::Global>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/slice.rs:111:9: 111:28
    = note: inside `std::slice::<impl [u8]>::to_vec_in::<std::alloc::Global>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/slice.rs:441:9: 441:34
    = note: inside `std::slice::<impl [u8]>::to_vec` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/slice.rs:416:9: 416:31
    = note: inside `std::slice::<impl std::borrow::ToOwned for [u8]>::to_owned` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/slice.rs:823:9: 823:22
    = note: inside `std::str::<impl std::borrow::ToOwned for str>::to_owned` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/str.rs:209:46: 209:72
    = note: inside `std::borrow::Cow::<'_, str>::into_owned` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/borrow.rs:325:35: 325:54
    = note: inside closure at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:146:33: 146:50
    = note: inside `swc_atoms::hstr::dynamic::no_inline_wrap::<{closure@<&mut swc_atoms::hstr::AtomStore as swc_atoms::hstr::dynamic::Storage>::insert_entry::{closure#1}}, std::sync::Arc<swc_atoms::hstr::dynamic::Entry>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:174:5: 174:9
    = note: inside `<&mut swc_atoms::hstr::AtomStore as swc_atoms::hstr::dynamic::Storage>::insert_entry` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:144:25: 151:19
    = note: inside `swc_atoms::hstr::dynamic::new_atom::<&mut swc_atoms::hstr::AtomStore>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:107:17: 107:49
    = note: inside `swc_atoms::hstr::AtomStore::atom::<'_, std::borrow::Cow<'_, str>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hstr-0.2.6/src/dynamic.rs:82:9: 82:36
    = note: inside `swc_atoms::AtomStore::atom::<'_, std::borrow::Cow<'_, str>>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.5/src/lib.rs:230:14: 230:28
    = note: inside `swc_atoms::AtomStoreCell::atom::<'_, &str>` at /home/lm978/.cargo/registry/src/index.crates.io-6f17d22bba15001f/swc_atoms-0.6.5/src/lib.rs:247:18: 247:41
note: inside closure
   --> src/main.rs:33:9
    |
33  |         store.atom("example");
    |         ^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::thread::LocalKey::<swc_atoms::AtomStoreCell>::try_with::<{closure@src/main.rs:31:16: 31:23}, ()>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:270:16: 270:31
    = note: inside `std::thread::LocalKey::<swc_atoms::AtomStoreCell>::with::<{closure@src/main.rs:31:16: 31:23}, ()>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:246:9: 246:25
note: inside `additional_work`
   --> src/main.rs:31:5
    |
31  | /     STORE.with(|store| {
32  | |         ALLOC.0.store(true, Ordering::Relaxed);
33  | |         store.atom("example");
34  | |     });
    | |______^
note: inside `<EvilAlloc as std::alloc::GlobalAlloc>::alloc`
   --> src/main.rs:14:13
    |
14  |             additional_work();
    |             ^^^^^^^^^^^^^^^^^
note: inside `_::__rust_alloc`
   --> src/main.rs:8:15
    |
7   | #[global_allocator]
    | ------------------- in this procedural macro expansion
8   | static ALLOC: EvilAlloc = EvilAlloc(AtomicBool::new(false));
    |               ^^^^^^^^^
    = note: inside `std::alloc::alloc` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:98:9: 98:52
    = note: inside `std::alloc::Global::alloc_impl` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:181:73: 181:86
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:241:9: 241:39
    = note: inside `alloc::alloc::exchange_malloc` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:330:11: 330:34
    = note: inside `std::boxed::Box::<i32>::new` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:217:9: 217:20
note: inside `main`
   --> src/main.rs:39:15
    |
39  |     Box::leak(Box::new(0));
    |               ^^^^^^^^^^^
    = note: this error originates in the attribute macro `global_allocator` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Notice how the allocator is re-entered when <&mut AtomStore as Storage>::insert_entry() calls Cow::into_owned() on the string passed in.

[kdy1]

It's UB caused by the user's allocator, not AtomStoreCell

[LegionMammal978]

It's UB caused by the user's allocator, not AtomStoreCell

The point is that user-defined allocators have absolutely no safety obligations regarding reentering other safe APIs; their one and only obligation is to return a valid block of memory from alloc(), and to keep it valid until freed. Indeed, with the unstable alloc_error_hook feature, the function can be reentered with no unsafe code at all:

// 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");
    });
}

start of outer call
start of inner call
end of inner call
Aborted (core dumped)

Note how the inner call starts while the outer call is still running; this creates two aliasing &mut references.

[workingjubilee]

It's not clear that "don't fuck up and make an allocator that reenters itself" isn't also an obligation of GlobalAlloc, just implicit. However, the safe example you demonstrated motivates at least a discussion of that nightly feature. Would you open an issue regarding that example with set_alloc_error_hook against rust-lang/rust, please?

[RalfJung]

However, the safe example you demonstrated motivates at least a discussion of that nightly feature. Would you open an issue regarding that example with set_alloc_error_hook against rust-lang/rust, please?

#[alloc_error_handler] is stable and has the same issue, I think?

[workingjubilee]

That is not quite the case, it was reverted from stabilization and rust-lang/rust#112331 is the current PR for handling that.

Note however that this remains an issue with panic hooks if any of the code inside AtomStoreCell panics.

[swc-bot]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.