From 8c9cb06c2ec287e4b9d2bce79390b444752c3686 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 10 Jul 2020 23:53:25 +0200 Subject: [PATCH 1/5] Deny unsafe op in unsafe fns without the unsafe keyword, first part for std/thread --- library/std/src/thread/local.rs | 99 +++++++++++++++++++++++---------- library/std/src/thread/mod.rs | 28 +++++++--- 2 files changed, 91 insertions(+), 36 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 60a05dc5d545b..991631d91f0b2 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -288,7 +288,11 @@ mod lazy { } pub unsafe fn get(&self) -> Option<&'static T> { - (*self.inner.get()).as_ref() + // SAFETY: 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() } } pub unsafe fn initialize T>(&self, init: F) -> &'static T { @@ -297,6 +301,8 @@ mod lazy { 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 +315,31 @@ 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(), + unsafe { + let _ = mem::replace(&mut *ptr, Some(value)); + } + + // SAFETY: the *ptr operation is made safe by the `mem::replace` + // call above that made sure a valid value is present behind it. + 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(), + } } } #[allow(unused)] pub unsafe fn take(&mut self) -> Option { - (*self.inner.get()).take() + // SAFETY: The other methods hand out references while taking &self. + // As such, calling this method when such references are still alive + // will fail because it takes a &mut self, conflicting with them. + unsafe { (*self.inner.get()).take() } } } } @@ -413,9 +428,17 @@ 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 call to `get` is made safe because no mutable references are + // ever handed out and the `try_initialize` is dependant on the + // passed `init` function. + unsafe { + match self.inner.get() { + Some(val) => Some(val), + None => self.try_initialize(init), + } } } @@ -428,8 +451,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. + if !mem::needs_drop::() || unsafe { self.try_register_dtor() } { + // SAFETY: See comment above. + Some(unsafe { self.inner.initialize(init) }) } else { None } @@ -441,8 +466,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 +487,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); + } } } @@ -533,11 +570,15 @@ pub mod os { 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 +586,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), })) From a83b79ec31df3a467753c3e5b41cf457af544c7b Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sat, 11 Jul 2020 00:15:24 +0200 Subject: [PATCH 2/5] Finished documenting all unsafe op inside unsafe fn --- library/std/src/thread/local.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 991631d91f0b2..281e1ef6741a5 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -539,20 +539,28 @@ pub mod os { } pub unsafe fn get(&'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 { - 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; @@ -563,7 +571,11 @@ 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 From 5d29954b2f2c3e079372bbaaee2ed64c1674046b Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sat, 29 Aug 2020 20:10:05 +0200 Subject: [PATCH 3/5] Improve some SAFETY comments following suggestions --- library/std/src/thread/local.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 281e1ef6741a5..54efff2a92bbf 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -315,12 +315,23 @@ 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. + // + // `ptr` can be dereferenced safely since it was obtained from + // `UnsafeCell::get`, which should not return a non-aligned or NUL pointer. + // What's more a `LazyKeyInner` can only be created with `new`, which ensures + // `inner` is correctly initialized and all calls to methods on `LazyKeyInner` + // will leave `inner` initialized too. unsafe { let _ = mem::replace(&mut *ptr, Some(value)); } - // SAFETY: the *ptr operation is made safe by the `mem::replace` - // call above that made sure a valid value is present behind it. + // SAFETY: the `*ptr` operation is made safe by the `mem::replace` + // call above combined with `ptr` being correct from the beginning + // (see previous SAFETY: comment above). + // + // Plus, 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 @@ -337,8 +348,8 @@ mod lazy { #[allow(unused)] pub unsafe fn take(&mut self) -> Option { // SAFETY: The other methods hand out references while taking &self. - // As such, calling this method when such references are still alive - // will fail because it takes a &mut self, conflicting with them. + // As such, callers of this method must ensure no `&` and `&mut` are + // available and used at the same time. unsafe { (*self.inner.get()).take() } } } @@ -451,9 +462,9 @@ 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> { - // SAFETY: See comment above. + // SAFETY: See comment above (this function doc). if !mem::needs_drop::() || unsafe { self.try_register_dtor() } { - // SAFETY: See comment above. + // SAFETY: See comment above (his function doc). Some(unsafe { self.inner.initialize(init) }) } else { None From 3afadaad4f087c86b1a8509109f544214ecad45f Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 20 Sep 2020 17:31:55 +0200 Subject: [PATCH 4/5] Fix accordingly to review --- library/std/src/thread/local.rs | 42 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 54efff2a92bbf..cc9cad2162b1b 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -288,13 +288,15 @@ mod lazy { } pub unsafe fn get(&self) -> Option<&'static T> { - // SAFETY: 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. + // 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. @@ -316,20 +318,13 @@ mod lazy { // destructor" flag we just use `mem::replace` which should sequence the // operations a little differently and make this safe to call. // - // `ptr` can be dereferenced safely since it was obtained from - // `UnsafeCell::get`, which should not return a non-aligned or NUL pointer. - // What's more a `LazyKeyInner` can only be created with `new`, which ensures - // `inner` is correctly initialized and all calls to methods on `LazyKeyInner` - // will leave `inner` initialized too. + // 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: the `*ptr` operation is made safe by the `mem::replace` - // call above combined with `ptr` being correct from the beginning - // (see previous SAFETY: comment above). - // - // Plus, with the call to `mem::replace` it is guaranteed there is + // 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 { @@ -345,11 +340,12 @@ mod lazy { } } + /// 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 { - // SAFETY: 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. + // SAFETY: See doc comment for this method. unsafe { (*self.inner.get()).take() } } } @@ -442,9 +438,10 @@ pub mod fast { // SAFETY: See the definitions of `LazyKeyInner::get` and // `try_initialize` for more informations. // - // The call to `get` is made safe because no mutable references are - // ever handed out and the `try_initialize` is dependant on the - // passed `init` function. + // 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), @@ -549,9 +546,10 @@ 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> { - // SAFETY: No mutable references are ever handed out meaning getting - // the value is ok. + // SAFETY: See the documentation for this method. let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize > 1 { // SAFETY: the check ensured the pointer is safe (its destructor From d01bd19573a14d53a035bae704bdcdab0680a283 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Mon, 21 Sep 2020 23:08:48 +0200 Subject: [PATCH 5/5] Fix missing unsafe block for target arch wasm32 --- library/std/src/thread/local.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index cc9cad2162b1b..7226692fa0c98 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -377,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) } }