Using `thread_local!` with `Drop` from `GlobalAlloc::alloc` causes crashes

[nvzqz]

I tried this code:

use std::alloc::{GlobalAlloc, Layout, System};

#[global_allocator]
static ALLOC: Alloc = Alloc;

struct Alloc;

struct Local;

impl Drop for Local {
    fn drop(&mut self) {}
}

thread_local! {
    static LOCAL: Local = Local;
}

unsafe impl GlobalAlloc for Alloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        LOCAL.with(|_local| {});
        System.alloc(layout)
    }

    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        System.dealloc(ptr, layout)
    }
}

fn main() {
    println!("Hello, world!");
}

I expected to see this happen: it prints Hello, world!.

Instead, this happened: terminated by signal SIGILL (Illegal instruction)

When run under Miri, it reports undefined behavior:

error: Undefined Behavior: not granting access to tag <715> because that would remove [Unique for <729>] which is strongly protected because it is an argument of call 293
  --> /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/thread_local_dtor.rs:72:16
   |
72 |     let list = &mut DTORS;
   |                ^^^^^^^^^^ not granting access to tag <715> because that would remove [Unique for <729>] which is strongly protected because it is an argument of call 293
   |
   = 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: <715> was created here, as the base tag for alloc381
  --> src/main.rs:14:1
   |
14 | / thread_local! {
15 | |     static LOCAL: Local = Local;
16 | | }
   | |_^
help: <729> is this argument
  --> src/main.rs:14:1
   |
14 | / thread_local! {
15 | |     static LOCAL: Local = Local;
16 | | }
   | |_^
   = note: BACKTRACE (of the first span):
   = note: inside `std::sys::unix::thread_local_dtor::register_dtor` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/thread_local_dtor.rs:72:16: 72:26
   = note: inside `std::thread::local_impl::Key::<Local>::try_register_dtor` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:207:26: 207:88
   = note: inside `std::thread::local_impl::Key::<Local>::try_initialize::<[closure@/Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:91:31: 91:38]>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:188:48: 188:72
   = note: inside `std::thread::local_impl::Key::<Local>::get::<[closure@/Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:91:31: 91:38]>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:173:25: 173:50
note: inside `LOCAL::__getit`
  --> src/main.rs:14:1
   |
14 | / thread_local! {
15 | |     static LOCAL: Local = Local;
16 | | }
   | |_^
   = note: inside `std::thread::LocalKey::<Local>::try_with::<[closure@src/main.rs:20:20: 20:28], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:269:32: 269:50
   = note: inside `std::thread::LocalKey::<Local>::with::<[closure@src/main.rs:20:20: 20:28], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:246:9: 246:25
note: inside `<Alloc as std::alloc::GlobalAlloc>::alloc`
  --> src/main.rs:20:9
   |
20 |         LOCAL.with(|_local| {});
   |         ^^^^^^^^^^^^^^^^^^^^^^^
note: inside `_::__rust_alloc`
  --> src/main.rs:4:15
   |
3  | #[global_allocator]
   | ------------------- in this procedural macro expansion
4  | static ALLOC: Alloc = Alloc;
   |               ^^^^^
   = note: inside `std::alloc::alloc` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:98:9: 98:52
   = note: inside `std::alloc::Global::alloc_impl` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:181:73: 181:86
   = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:241:9: 241:39
   = note: inside `alloc::raw_vec::finish_grow::<std::alloc::Global>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:485:9: 485:35
   = note: inside `alloc::raw_vec::RawVec::<(*mut u8, unsafe extern "C" fn(*mut u8))>::grow_amortized` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:404:19: 404:82
   = note: inside `alloc::raw_vec::RawVec::<(*mut u8, unsafe extern "C" fn(*mut u8))>::reserve_for_push` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:302:24: 302:51
   = note: inside `std::vec::Vec::<(*mut u8, unsafe extern "C" fn(*mut u8))>::push` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:1829:13: 1829:48
   = note: inside `std::sys::unix::thread_local_dtor::register_dtor` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/thread_local_dtor.rs:73:5: 73:25
   = note: inside `std::thread::local_impl::Key::<Local>::try_register_dtor` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:207:26: 207:88
   = note: inside `std::thread::local_impl::Key::<Local>::try_initialize::<[closure@/Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:91:31: 91:38]>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:188:48: 188:72
   = note: inside `std::thread::local_impl::Key::<Local>::get::<[closure@/Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:91:31: 91:38]>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/common/thread_local/fast_local.rs:173:25: 173:50
note: inside `LOCAL::__getit`
  --> src/main.rs:14:1
   |
14 | / thread_local! {
15 | |     static LOCAL: Local = Local;
16 | | }
   | |_^
   = note: inside `std::thread::LocalKey::<Local>::try_with::<[closure@src/main.rs:20:20: 20:28], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:269:32: 269:50
   = note: inside `std::thread::LocalKey::<Local>::with::<[closure@src/main.rs:20:20: 20:28], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:246:9: 246:25
note: inside `<Alloc as std::alloc::GlobalAlloc>::alloc`
  --> src/main.rs:20:9
   |
20 |         LOCAL.with(|_local| {});
   |         ^^^^^^^^^^^^^^^^^^^^^^^
note: inside `_::__rust_alloc`
  --> src/main.rs:4:15
   |
3  | #[global_allocator]
   | ------------------- in this procedural macro expansion
4  | static ALLOC: Alloc = Alloc;
   |               ^^^^^
   = note: inside `std::alloc::alloc` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:98:9: 98:52
   = note: inside `std::alloc::Global::alloc_impl` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:181:73: 181:86
   = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/alloc.rs:241:9: 241:39
   = note: inside `alloc::raw_vec::RawVec::<u8>::allocate_in` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/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 /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:130:9: 130:69
   = note: inside `std::vec::Vec::<u8>::with_capacity_in` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:670:20: 670:61
   = note: inside `std::vec::Vec::<u8>::with_capacity` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:479:9: 479:49
   = note: inside `std::ffi::CString::new::spec_new_impl_bytes` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/ffi/c_str.rs:287:30: 287:58
   = note: inside `<&str as std::ffi::CString::new::SpecNewImpl>::spec_new_impl` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/ffi/c_str.rs:306:17: 306:53
   = note: inside `std::ffi::CString::new::<&str>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/ffi/c_str.rs:316:9: 316:26
   = note: inside `std::rt::init` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:104:53: 104:73
   = note: inside closure at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:147:42: 147:67
   = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:524:40: 524:43
   = note: inside `std::panicking::r#try::<(), [closure@std::rt::lang_start_internal::{closure#1}]>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:488:19: 488:81
   = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:142:14: 142:33
   = note: inside `std::rt::lang_start_internal` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:147:5: 147:70
   = note: inside `std::rt::lang_start::<()>` at /Users/nv/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:165:17: 170:6
   = note: this error originates in the macro `$crate::thread::local_impl::thread_local_inner` which comes from the expansion of the attribute macro `global_allocator` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

It appears the UB is caused by register_dtor calling Vec::push and thus getting multiple mutable references simultaneously.

Meta

rustc --version --verbose:

rustc 1.72.1 (d5c2e9c34 2023-09-13)
binary: rustc
commit-hash: d5c2e9c342b358556da91d61ed4133f6f50fc0c3
commit-date: 2023-09-13
host: aarch64-apple-darwin
release: 1.72.1
LLVM version: 16.0.5

rustc +nightly --version --verbose:

rustc 1.75.0-nightly (2e5a9dd6c 2023-10-02)
binary: rustc
commit-hash: 2e5a9dd6c9eaa42f0684b4b760bd68fc27cbe51b
commit-date: 2023-10-02
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.2

[nvzqz]

I created a workaround for my needs where I instead access the Drop thread-local in GlobalAlloc::dealloc once per-thread by checking another non-Drop thread-local.

This seems to be Mac-specific looking through the code, and neither am I able to reproduce this on Linux.

[Jules-Bertholet]

@rustbot label A-allocators I-unsound

[joboet]

I don't think we'll be able to avoid crashes in this situation entirely. TLS fundamentally needs to allocate on some platforms, so unless we unconditionally use the system allocator for this, there will always be crashes here. In #116402 I removed the UB part of this (I hope), so after merging that, this will probably be more of a documentation problem.

This seems to fall in the same category as #110708; it's another edge-case where stds assumptions fall apart.

[nvzqz]

(Repeating for visibility) I think an ideal long term solution would be to make register_dtor reentrant, so that global allocators can register TLS destructors.

It's not uncommon for custom allocators to take advantage of thread local storage to improve performance. My personal use case is to efficiently profile allocations by keeping counters specific to each thread.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

As per this comment, probably fixed by #116402 (at least the UB part).

@rustbot label -I-prioritize +P-high