diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 60a05dc5d545b..7226692fa0c98 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -288,15 +288,23 @@ mod lazy { } pub unsafe fn get(&self) -> Option<&'static T> { - (*self.inner.get()).as_ref() + // SAFETY: The caller must ensure no reference is ever handed out to + // the inner cell nor mutable reference to the Option inside said + // cell. This make it safe to hand a reference, though the lifetime + // of 'static is itself unsafe, making the get method unsafe. + unsafe { (*self.inner.get()).as_ref() } } + /// The caller must ensure that no reference is active: this method + /// needs unique access. pub unsafe fn initialize T>(&self, init: F) -> &'static T { // Execute the initialization up front, *then* move it into our slot, // just in case initialization fails. let value = init(); let ptr = self.inner.get(); + // SAFETY: + // // note that this can in theory just be `*ptr = Some(value)`, but due to // the compiler will currently codegen that pattern with something like: // @@ -309,22 +317,36 @@ mod lazy { // value (an aliasing violation). To avoid setting the "I'm running a // destructor" flag we just use `mem::replace` which should sequence the // operations a little differently and make this safe to call. - let _ = mem::replace(&mut *ptr, Some(value)); - - // After storing `Some` we want to get a reference to the contents of - // what we just stored. While we could use `unwrap` here and it should - // always work it empirically doesn't seem to always get optimized away, - // which means that using something like `try_with` can pull in - // panicking code and cause a large size bloat. - match *ptr { - Some(ref x) => x, - None => hint::unreachable_unchecked(), + // + // The precondition also ensures that we are the only one accessing + // `self` at the moment so replacing is fine. + unsafe { + let _ = mem::replace(&mut *ptr, Some(value)); + } + + // SAFETY: With the call to `mem::replace` it is guaranteed there is + // a `Some` behind `ptr`, not a `None` so `unreachable_unchecked` + // will never be reached. + unsafe { + // After storing `Some` we want to get a reference to the contents of + // what we just stored. While we could use `unwrap` here and it should + // always work it empirically doesn't seem to always get optimized away, + // which means that using something like `try_with` can pull in + // panicking code and cause a large size bloat. + match *ptr { + Some(ref x) => x, + None => hint::unreachable_unchecked(), + } } } + /// The other methods hand out references while taking &self. + /// As such, callers of this method must ensure no `&` and `&mut` are + /// available and used at the same time. #[allow(unused)] pub unsafe fn take(&mut self) -> Option { - (*self.inner.get()).take() + // SAFETY: See doc comment for this method. + unsafe { (*self.inner.get()).take() } } } } @@ -355,10 +377,17 @@ pub mod statik { } pub unsafe fn get(&self, init: fn() -> T) -> Option<&'static T> { - let value = match self.inner.get() { - Some(ref value) => value, - None => self.inner.initialize(init), + // SAFETY: The caller must ensure no reference is ever handed out to + // the inner cell nor mutable reference to the Option inside said + // cell. This make it safe to hand a reference, though the lifetime + // of 'static is itself unsafe, making the get method unsafe. + let value = unsafe { + match self.inner.get() { + Some(ref value) => value, + None => self.inner.initialize(init), + } }; + Some(value) } } @@ -413,9 +442,18 @@ pub mod fast { } pub unsafe fn get T>(&self, init: F) -> Option<&'static T> { - match self.inner.get() { - Some(val) => Some(val), - None => self.try_initialize(init), + // SAFETY: See the definitions of `LazyKeyInner::get` and + // `try_initialize` for more informations. + // + // The caller must ensure no mutable references are ever active to + // the inner cell or the inner T when this is called. + // The `try_initialize` is dependant on the passed `init` function + // for this. + unsafe { + match self.inner.get() { + Some(val) => Some(val), + None => self.try_initialize(init), + } } } @@ -428,8 +466,10 @@ pub mod fast { // LLVM issue: https://bugs.llvm.org/show_bug.cgi?id=41722 #[inline(never)] unsafe fn try_initialize T>(&self, init: F) -> Option<&'static T> { - if !mem::needs_drop::() || self.try_register_dtor() { - Some(self.inner.initialize(init)) + // SAFETY: See comment above (this function doc). + if !mem::needs_drop::() || unsafe { self.try_register_dtor() } { + // SAFETY: See comment above (his function doc). + Some(unsafe { self.inner.initialize(init) }) } else { None } @@ -441,8 +481,12 @@ pub mod fast { unsafe fn try_register_dtor(&self) -> bool { match self.dtor_state.get() { DtorState::Unregistered => { - // dtor registration happens before initialization. - register_dtor(self as *const _ as *mut u8, destroy_value::); + // SAFETY: dtor registration happens before initialization. + // Passing `self` as a pointer while using `destroy_value` + // is safe because the function will build a pointer to a + // Key, which is the type of self and so find the correct + // size. + unsafe { register_dtor(self as *const _ as *mut u8, destroy_value::) }; self.dtor_state.set(DtorState::Registered); true } @@ -458,13 +502,21 @@ pub mod fast { unsafe extern "C" fn destroy_value(ptr: *mut u8) { let ptr = ptr as *mut Key; + // SAFETY: + // + // The pointer `ptr` has been built just above and comes from + // `try_register_dtor` where it is originally a Key coming from `self`, + // making it non-NUL and of the correct type. + // // Right before we run the user destructor be sure to set the // `Option` to `None`, and `dtor_state` to `RunningOrHasRun`. This // causes future calls to `get` to run `try_initialize_drop` again, // which will now fail, and return `None`. - let value = (*ptr).inner.take(); - (*ptr).dtor_state.set(DtorState::RunningOrHasRun); - drop(value); + unsafe { + let value = (*ptr).inner.take(); + (*ptr).dtor_state.set(DtorState::RunningOrHasRun); + drop(value); + } } } @@ -501,21 +553,30 @@ pub mod os { Key { os: OsStaticKey::new(Some(destroy_value::)), marker: marker::PhantomData } } + /// It is a requirement for the caller to ensure that no mutable + /// reference is active when this method is called. pub unsafe fn get(&'static self, init: fn() -> T) -> Option<&'static T> { - let ptr = self.os.get() as *mut Value; + // SAFETY: See the documentation for this method. + let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize > 1 { - if let Some(ref value) = (*ptr).inner.get() { + // SAFETY: the check ensured the pointer is safe (its destructor + // is not running) + it is coming from a trusted source (self). + if let Some(ref value) = unsafe { (*ptr).inner.get() } { return Some(value); } } - self.try_initialize(init) + // SAFETY: At this point we are sure we have no value and so + // initializing (or trying to) is safe. + unsafe { self.try_initialize(init) } } // `try_initialize` is only called once per os thread local variable, // except in corner cases where thread_local dtors reference other // thread_local's, or it is being recursively initialized. unsafe fn try_initialize(&'static self, init: fn() -> T) -> Option<&'static T> { - let ptr = self.os.get() as *mut Value; + // SAFETY: No mutable references are ever handed out meaning getting + // the value is ok. + let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize == 1 { // destructor is running return None; @@ -526,18 +587,26 @@ pub mod os { // local copy, so do that now. let ptr: Box> = box Value { inner: LazyKeyInner::new(), key: self }; let ptr = Box::into_raw(ptr); - self.os.set(ptr as *mut u8); + // SAFETY: At this point we are sure there is no value inside + // ptr so setting it will not affect anyone else. + unsafe { + self.os.set(ptr as *mut u8); + } ptr } else { // recursive initialization ptr }; - Some((*ptr).inner.initialize(init)) + // SAFETY: ptr has been ensured as non-NUL just above an so can be + // dereferenced safely. + unsafe { Some((*ptr).inner.initialize(init)) } } } unsafe extern "C" fn destroy_value(ptr: *mut u8) { + // SAFETY: + // // The OS TLS ensures that this key contains a NULL value when this // destructor starts to run. We set it back to a sentinel value of 1 to // ensure that any future calls to `get` for this thread will return @@ -545,10 +614,12 @@ pub mod os { // // Note that to prevent an infinite loop we reset it back to null right // before we return from the destructor ourselves. - let ptr = Box::from_raw(ptr as *mut Value); - let key = ptr.key; - key.os.set(1 as *mut u8); - drop(ptr); - key.os.set(ptr::null_mut()); + unsafe { + let ptr = Box::from_raw(ptr as *mut Value); + let key = ptr.key; + key.os.set(1 as *mut u8); + drop(ptr); + key.os.set(ptr::null_mut()); + } } } diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 6d6be8560aa36..8c353e2484ef2 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -144,6 +144,7 @@ //! [`with`]: LocalKey::with #![stable(feature = "rust1", since = "1.0.0")] +#![deny(unsafe_op_in_unsafe_fn)] #[cfg(all(test, not(target_os = "emscripten")))] mod tests; @@ -456,14 +457,23 @@ impl Builder { imp::Thread::set_name(name); } - thread_info::set(imp::guard::current(), their_thread); + // SAFETY: the stack guard passed is the one for the current thread. + // This means the current thread's stack and the new thread's stack + // are properly set and protected from each other. + thread_info::set(unsafe { imp::guard::current() }, their_thread); let try_result = panic::catch_unwind(panic::AssertUnwindSafe(|| { crate::sys_common::backtrace::__rust_begin_short_backtrace(f) })); - *their_packet.get() = Some(try_result); + // SAFETY: `their_packet` as been built just above and moved by the + // closure (it is an Arc<...>) and `my_packet` will be stored in the + // same `JoinInner` as this closure meaning the mutation will be + // safe (not modify it and affect a value far away). + unsafe { *their_packet.get() = Some(try_result) }; }; Ok(JoinHandle(JoinInner { + // SAFETY: + // // `imp::Thread::new` takes a closure with a `'static` lifetime, since it's passed // through FFI or otherwise used with low-level threading primitives that have no // notion of or way to enforce lifetimes. @@ -475,12 +485,14 @@ impl Builder { // Similarly, the `sys` implementation must guarantee that no references to the closure // exist after the thread has terminated, which is signaled by `Thread::join` // returning. - native: Some(imp::Thread::new( - stack_size, - mem::transmute::, Box>(Box::new( - main, - )), - )?), + native: unsafe { + Some(imp::Thread::new( + stack_size, + mem::transmute::, Box>( + Box::new(main), + ), + )?) + }, thread: my_thread, packet: Packet(my_packet), }))