Repository navigation
std::thread::local internals allow race conditions in safe but unstable code. #43733
Description
#![feature(const_fn, drop_types_in_const, thread_local_internals)]
type Foo = std::cell::RefCell<String>;
static __KEY: std::thread::__FastLocalKeyInner<Foo> =
std::thread::__FastLocalKeyInner::new();
fn __getit() -> std::option::Option<
&'static std::cell::UnsafeCell<
std::option::Option<Foo>>>
{
__KEY.get()
}
static FOO: std::thread::LocalKey<Foo> =
std::thread::LocalKey::new(__getit, Default::default);
fn main() {
FOO.with(|foo| println!("{}", foo.borrow()));
std::thread::spawn(|| {
FOO.with(|foo| *foo.borrow_mut() += "foo");
}).join().unwrap();
FOO.with(|foo| println!("{}", foo.borrow()));
}Discovered while working on a sound fix for #17954. The problem is this decision which prevents the thread_local! macro from using unsafe to state that it actually has a #[thread_local] static to use in LocalKey. Without that, anything of 'static lifetime (which is btw quite wrong for #[thread_local]) and the right type can be used, from safe code.
While we can change the APIs all we want, going back to the correct solution of #[allow(unsafe_code)] could be a breaking change, without unsafety hygiene.
cc @rust-lang/libs
Activity
eddyb commentedon Aug 8, 2017
cc @nikomatsakis @arielb1 @Manishearth
Manishearth commentedon Aug 8, 2017
We can probably introduce a way for
#[allow]to punch throughforbidthe same wayallow_internal_unstabledoes. But it might be tricky.eddyb commentedon Aug 8, 2017
@Manishearth Exactly my thoughts but it'd be nice if we had
unsafehygiene for this.However, if
#[allow_internal_unsafe]is fine for this, it would not be hard to implement.Manishearth commentedon Aug 8, 2017
A slight hole in that would be that now user-written unsafe code in
thread_local!()won't be caught by the lint........ I'm not sure what to do about that.
deny(unsafe)/forbid(unsafe)is something I'd rather keep as sound as possible (yes, there are ways to circumvent it, but assuming a "non-malicious but non-smart" actor -- i.e. all of us programmers writing terrible code -- it's nice when these things are perfect)eddyb commentedon Aug 8, 2017
@Manishearth
#[allow_internal_unstable]already doesn't have that issue, it's hygienic.Manishearth commentedon Aug 8, 2017
Oh, right!
eddyb commentedon Aug 8, 2017
@arielb1 suggested using item hygiene from the new macros. So I added
#[feature(macro_decl)]tolibstd, changed__thread_local_innerto be apub macroitem and I got 275 missing stability, documentation, etc. errors, because private items now "appear to be" macro-accessible.It's reproducible with just
pub macro foo() {}insrc/libstd/lib.rs.So
#[allow_internal_unstable]remains the only way forward for now.est31 commentedon Aug 8, 2017
rust-lang/rfcs#1705
eddyb commentedon Aug 8, 2017
@est31
'threadis unsound/uncheckable. See sound fix at #43746.joboet commentedon Sep 25, 2023
The regression test for this issue caused build failures for me in #116123. Instead of trying to rework it to fit the new API, is everyone involved here fine with me removing it? IIUC, it only tests unstable, internal API of
std, which is being reworked as part of #110897 anyway.