Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Using thread_local! with Drop from GlobalAlloc::alloc causes crashes #116390

Open
nvzqz opened this issue Oct 3, 2023 · 7 comments
Open

Using thread_local! with Drop from GlobalAlloc::alloc causes crashes #116390

nvzqz opened this issue Oct 3, 2023 · 7 comments
Labels
A-allocators Area: Custom and system allocators C-bug Category: This is a bug. P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@nvzqz
Copy link
Contributor

nvzqz commented Oct 3, 2023 •
edited
Loading

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 nvzqz added the C-bug Category: This is a bug. label Oct 3, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 3, 2023
@nvzqz nvzqz changed the title Using thread_local! from GlobalAlloc causes crash and undefined behavior Using thread_local! with Drop from GlobalAlloc causes crash and undefined behavior Oct 3, 2023
@nvzqz
Copy link
Contributor Author

nvzqz commented Oct 3, 2023

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.

@nvzqz nvzqz changed the title Using thread_local! with Drop from GlobalAlloc causes crash and undefined behavior Using thread_local! with Drop from GlobalAlloc::alloc causes crash and undefined behavior Oct 3, 2023
@gimbling-away
Copy link
Contributor

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

@Jules-Bertholet
Copy link
Contributor

@rustbot label A-allocators I-unsound

@rustbot rustbot added A-allocators Area: Custom and system allocators I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 3, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 3, 2023
@joboet joboet mentioned this issue Oct 3, 2023
Merged
@joboet
Copy link
Member

joboet commented Oct 3, 2023

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
Copy link
Contributor Author

nvzqz commented Oct 3, 2023

(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
Copy link
Contributor

apiraino commented Oct 4, 2023

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

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 4, 2023
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 4, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 5, 2023
@matthiaskrgr
Rollup merge of rust-lang#116402 - joboet:global_alloc_tls_unsoundnes…
8be8c40 
…s, r=thomcc

Panic when the global allocator tries to register a TLS destructor

Using a `RefCell` avoids the undefined behaviour encountered in rust-lang#116390 and reduces the amount of `unsafe` code in the codebase.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 5, 2023
@bors
Auto merge of rust-lang#116402 - joboet:global_alloc_tls_unsoundness,…
a64a576 
… r=thomcc

Panic when the global allocator tries to register a TLS destructor

Using a `RefCell` avoids the undefined behaviour encountered in rust-lang#116390 and reduces the amount of `unsafe` code in the codebase.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 9, 2023
@bors
Auto merge of rust-lang#116402 - joboet:global_alloc_tls_unsoundness,…
8572233 
… r=thomcc,workingjubilee

Panic when the global allocator tries to register a TLS destructor

Using a `RefCell` avoids the undefined behaviour encountered in rust-lang#116390 and reduces the amount of `unsafe` code in the codebase.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 10, 2023
@bors
Auto merge of rust-lang#116402 - joboet:global_alloc_tls_unsoundness,…
982b101 
… r=thomcc,workingjubilee

Panic when the global allocator tries to register a TLS destructor

Using a `RefCell` avoids the undefined behaviour encountered in rust-lang#116390 and reduces the amount of `unsafe` code in the codebase.
@joboet joboet mentioned this issue Oct 17, 2023
Closed
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 19, 2023
@bors
Auto merge of rust-lang#116402 - joboet:global_alloc_tls_unsoundness,…
89432aa 
… r=thomcc,workingjubilee

Panic when the global allocator tries to register a TLS destructor

Using a `RefCell` avoids the undefined behaviour encountered in rust-lang#116390 and reduces the amount of `unsafe` code in the codebase.
@joboet
Copy link
Member

joboet commented Oct 21, 2023

#116402 is merged, so this is no longer unsound, the code now just crashes.

@rustbot label -I-unsound

@rustbot rustbot removed the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Oct 21, 2023
@joboet joboet changed the title Using thread_local! with Drop from GlobalAlloc::alloc causes crash and undefined behavior Using thread_local! with Drop from GlobalAlloc::alloc causes crashes Mar 12, 2024
@lolbinarycat lolbinarycat mentioned this issue Jul 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-allocators Area: Custom and system allocators C-bug Category: This is a bug. P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@apiraino @nvzqz @saethlin @joboet @rustbot @Jules-Bertholet @gimbling-away