From 4d3cffa3cfff646fe707363ef9f2bd10f0d0bca7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Jun 2015 13:57:27 -0700 Subject: [PATCH] std: Fix unsoundness of std::thread::ScopedKey Currently the compiler has no knowledge of `#[thread_local]` which forces users to take on two burdens of unsafety: * The lifetime of the borrow of a `#[thread_local]` static is **not** `'static` * Types in `static`s are required to be `Sync` The thread-local modules mostly curb these facets of unsafety by only allowing very limited scopes of borrows as well as allowing all types to be stored in a thread-local key (regardless of whether they are `Sync`) through an `unsafe impl`. Unfortunately these measures have the consequence of being able to take the address of the key itself and send it to another thread, allowing the same key to be accessed from two different threads. This is clearly unsafe, and this commit fixes this problem with the same trick used by `LocalKey`, which is to have an indirect function call to find the address of the *current thread's* thread local. This way the address of thread local keys can safely be sent among threads as their lifetime truly is `'static`. This commit will reduce the performance of cross-crate scoped thread locals as it now requires an indirect function call, but this can likely be overcome in a future commit. Closes #25894 --- src/libstd/thread/mod.rs | 1 + src/libstd/thread/scoped_tls.rs | 79 ++++++++++++++++++++------------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index f090d3e77ddfc..153b04360877b 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -217,6 +217,7 @@ pub use self::local::{LocalKey, LocalKeyState}; pub use self::scoped_tls::ScopedKey; #[doc(hidden)] pub use self::local::__KeyInner as __LocalKeyInner; +#[doc(hidden)] pub use self::scoped_tls::__KeyInner as __ScopedKeyInner; //////////////////////////////////////////////////////////////////////////////// // Builder diff --git a/src/libstd/thread/scoped_tls.rs b/src/libstd/thread/scoped_tls.rs index f0a4c318d9127..679902ec7abdb 100644 --- a/src/libstd/thread/scoped_tls.rs +++ b/src/libstd/thread/scoped_tls.rs @@ -43,6 +43,9 @@ use prelude::v1::*; +#[doc(hidden)] +pub use self::imp::KeyInner as __KeyInner; + /// Type representing a thread local storage key corresponding to a reference /// to the type parameter `T`. /// @@ -53,7 +56,7 @@ use prelude::v1::*; #[unstable(feature = "scoped_tls", reason = "scoped TLS has yet to have wide enough use to fully consider \ stabilizing its interface")] -pub struct ScopedKey { inner: imp::KeyInner } +pub struct ScopedKey { inner: fn() -> &'static imp::KeyInner } /// Declare a new scoped thread local storage key. /// @@ -64,42 +67,51 @@ pub struct ScopedKey { inner: imp::KeyInner } /// information. #[macro_export] #[allow_internal_unstable] -#[cfg(not(no_elf_tls))] macro_rules! scoped_thread_local { (static $name:ident: $t:ty) => ( - #[cfg_attr(not(any(windows, - target_os = "android", - target_os = "ios", - target_os = "openbsd", - target_arch = "aarch64")), - thread_local)] static $name: ::std::thread::ScopedKey<$t> = - ::std::thread::ScopedKey::new(); + __scoped_thread_local_inner!($t); ); (pub static $name:ident: $t:ty) => ( - #[cfg_attr(not(any(windows, - target_os = "android", - target_os = "ios", - target_os = "openbsd", - target_arch = "aarch64")), - thread_local)] pub static $name: ::std::thread::ScopedKey<$t> = - ::std::thread::ScopedKey::new(); + __scoped_thread_local_inner!($t); ); } +#[doc(hidden)] +#[unstable(feature = "thread_local_internals", + reason = "should not be necessary")] #[macro_export] #[allow_internal_unstable] #[cfg(no_elf_tls)] -macro_rules! scoped_thread_local { - (static $name:ident: $t:ty) => ( - static $name: ::std::thread::ScopedKey<$t> = - ::std::thread::ScopedKey::new(); - ); - (pub static $name:ident: $t:ty) => ( - pub static $name: ::std::thread::ScopedKey<$t> = - ::std::thread::ScopedKey::new(); - ); +macro_rules! __scoped_thread_local_inner { + ($t:ty) => {{ + static _KEY: ::std::thread::__ScopedKeyInner<$t> = + ::std::thread::__ScopedKeyInner::new(); + fn _getit() -> &'static ::std::thread::__ScopedKeyInner<$t> { &_KEY } + ::std::thread::ScopedKey::new(_getit) + }} +} + +#[doc(hidden)] +#[unstable(feature = "thread_local_internals", + reason = "should not be necessary")] +#[macro_export] +#[allow_internal_unstable] +#[cfg(not(no_elf_tls))] +macro_rules! __scoped_thread_local_inner { + ($t:ty) => {{ + #[cfg_attr(not(any(windows, + target_os = "android", + target_os = "ios", + target_os = "openbsd", + target_arch = "aarch64")), + thread_local)] + static _KEY: ::std::thread::__ScopedKeyInner<$t> = + ::std::thread::__ScopedKeyInner::new(); + fn _getit() -> &'static ::std::thread::__ScopedKeyInner<$t> { &_KEY } + ::std::thread::ScopedKey::new(_getit) + }} } #[unstable(feature = "scoped_tls", @@ -107,8 +119,8 @@ macro_rules! scoped_thread_local { stabilizing its interface")] impl ScopedKey { #[doc(hidden)] - pub const fn new() -> ScopedKey { - ScopedKey { inner: imp::KeyInner::new() } + pub const fn new(inner: fn() -> &'static imp::KeyInner) -> ScopedKey { + ScopedKey { inner: inner } } /// Inserts a value into this scoped thread local storage slot for a @@ -153,13 +165,14 @@ impl ScopedKey { } } + let inner = (self.inner)(); let prev = unsafe { - let prev = self.inner.get(); - self.inner.set(t as *const T as *mut T); + let prev = inner.get(); + inner.set(t as *const T as *mut T); prev }; - let _reset = Reset { key: &self.inner, val: prev }; + let _reset = Reset { key: inner, val: prev }; cb() } @@ -186,7 +199,7 @@ impl ScopedKey { F: FnOnce(&T) -> R { unsafe { - let ptr = self.inner.get(); + let ptr = (self.inner)().get(); assert!(!ptr.is_null(), "cannot access a scoped thread local \ variable without calling `set` first"); cb(&*ptr) @@ -195,7 +208,7 @@ impl ScopedKey { /// Test whether this TLS key has been `set` for the current thread. pub fn is_set(&'static self) -> bool { - unsafe { !self.inner.get().is_null() } + unsafe { !(self.inner)().get().is_null() } } } @@ -205,6 +218,7 @@ impl ScopedKey { target_os = "openbsd", target_arch = "aarch64", no_elf_tls)))] +#[doc(hidden)] mod imp { use std::cell::Cell; @@ -227,6 +241,7 @@ mod imp { target_os = "openbsd", target_arch = "aarch64", no_elf_tls))] +#[doc(hidden)] mod imp { use prelude::v1::*;