From bbcd3b2b139a94e5f892b157b361114ae67bbdf9 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 12 Jul 2020 23:45:49 +0200 Subject: [PATCH 1/2] Deny unsafe operations in unsafe fns in libstd/sync/ --- src/libstd/sync/mod.rs | 1 + src/libstd/sync/mpsc/blocking.rs | 8 +++++-- src/libstd/sync/mpsc/mod.rs | 12 ++++++++-- src/libstd/sync/mpsc/spsc_queue.rs | 35 +++++++++++++++++++----------- 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/libstd/sync/mod.rs b/src/libstd/sync/mod.rs index b6699910b07cf..5515c17581b34 100644 --- a/src/libstd/sync/mod.rs +++ b/src/libstd/sync/mod.rs @@ -149,6 +149,7 @@ //! [`Once`]: crate::sync::Once //! [`RwLock`]: crate::sync::RwLock +#![deny(unsafe_op_in_unsafe_fn)] #![stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")] diff --git a/src/libstd/sync/mpsc/blocking.rs b/src/libstd/sync/mpsc/blocking.rs index d34de6a4fac3e..2ae51ef404fac 100644 --- a/src/libstd/sync/mpsc/blocking.rs +++ b/src/libstd/sync/mpsc/blocking.rs @@ -47,14 +47,18 @@ impl SignalToken { /// flag. #[inline] pub unsafe fn cast_to_usize(self) -> usize { - mem::transmute(self.inner) + // SAFETY: this ok because of the internal represention of Arc, which + // is a pointer and phantom data (so the size of a pointer -> a usize). + unsafe { mem::transmute(self.inner) } } /// Converts from an unsafe usize value. Useful for retrieving a pipe's state /// flag. #[inline] pub unsafe fn cast_from_usize(signal_ptr: usize) -> SignalToken { - SignalToken { inner: mem::transmute(signal_ptr) } + // SAFETY: this ok because of the internal represention of Arc, which + // is a pointer and phantom data (so the size of a pointer -> a usize). + SignalToken { inner: unsafe { mem::transmute(signal_ptr) } } } } diff --git a/src/libstd/sync/mpsc/mod.rs b/src/libstd/sync/mpsc/mod.rs index d6cc811154f11..3a9e62f3ef99c 100644 --- a/src/libstd/sync/mpsc/mod.rs +++ b/src/libstd/sync/mpsc/mod.rs @@ -648,10 +648,18 @@ enum Flavor { trait UnsafeFlavor { fn inner_unsafe(&self) -> &UnsafeCell>; unsafe fn inner_mut(&self) -> &mut Flavor { - &mut *self.inner_unsafe().get() + // SAFETY: We are sure the inner value will never be NUL when this is + // called, the invariants of the module make it so. + // + // It is also ensured that no other (mutable) reference will be handed + // out while the one returned here is in action. + unsafe { &mut *self.inner_unsafe().get() } } unsafe fn inner(&self) -> &Flavor { - &*self.inner_unsafe().get() + // SAFETY: We are sure the inner value will never be NUL when this is + // called, the invariants of the module make it so nor will any mutable + // reference be active while this one is. + unsafe { &*self.inner_unsafe().get() } } } impl UnsafeFlavor for Sender { diff --git a/src/libstd/sync/mpsc/spsc_queue.rs b/src/libstd/sync/mpsc/spsc_queue.rs index 0274268f69f25..19dea7f24d179 100644 --- a/src/libstd/sync/mpsc/spsc_queue.rs +++ b/src/libstd/sync/mpsc/spsc_queue.rs @@ -99,7 +99,9 @@ impl Queue Self { let n1 = Node::new(); let n2 = Node::new(); - (*n1).next.store(n2, Ordering::Relaxed); + // SAFETY: At this point we know the pointer is not NUL since it was + // build just above. + unsafe { (*n1).next.store(n2, Ordering::Relaxed) } Queue { consumer: CacheAligned::new(Consumer { tail: UnsafeCell::new(n2), @@ -134,18 +136,25 @@ impl Queue *mut Node { // First try to see if we can consume the 'first' node for our uses. - if *self.producer.first.get() != *self.producer.tail_copy.get() { - let ret = *self.producer.first.get(); - *self.producer.0.first.get() = (*ret).next.load(Ordering::Relaxed); - return ret; - } - // If the above fails, then update our copy of the tail and try - // again. - *self.producer.0.tail_copy.get() = self.consumer.tail_prev.load(Ordering::Acquire); - if *self.producer.first.get() != *self.producer.tail_copy.get() { - let ret = *self.producer.first.get(); - *self.producer.0.first.get() = (*ret).next.load(Ordering::Relaxed); - return ret; + // SAFEY: Since `self` is a valid reference, accessing its inner values + // can be considered safe (without checking for NUL pointers). + // + // Dereferencing of `ret` are safe too because the pointer comes from + // `self` once again. + unsafe { + if *self.producer.first.get() != *self.producer.tail_copy.get() { + let ret = *self.producer.first.get(); + *self.producer.0.first.get() = (*ret).next.load(Ordering::Relaxed); + return ret; + } + // If the above fails, then update our copy of the tail and try + // again. + *self.producer.0.tail_copy.get() = self.consumer.tail_prev.load(Ordering::Acquire); + if *self.producer.first.get() != *self.producer.tail_copy.get() { + let ret = *self.producer.first.get(); + *self.producer.0.first.get() = (*ret).next.load(Ordering::Relaxed); + return ret; + } } // If all of that fails, then we have to allocate a new node // (there's nothing in the node cache). From 8ec744d2e7e85d0b92144118cd4bfb816987cc74 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Mon, 13 Jul 2020 22:54:18 +0200 Subject: [PATCH 2/2] Improve the SAFETY: comments and use the Arc::{into/from}_raw functions --- src/libstd/sync/mpsc/blocking.rs | 10 +++++----- src/libstd/sync/mpsc/mod.rs | 2 +- src/libstd/sync/mpsc/spsc_queue.rs | 15 ++++++--------- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/libstd/sync/mpsc/blocking.rs b/src/libstd/sync/mpsc/blocking.rs index 2ae51ef404fac..4cf45fa53b446 100644 --- a/src/libstd/sync/mpsc/blocking.rs +++ b/src/libstd/sync/mpsc/blocking.rs @@ -1,6 +1,5 @@ //! Generic support for building blocking abstractions. -use crate::mem; use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sync::Arc; use crate::thread::{self, Thread}; @@ -47,9 +46,7 @@ impl SignalToken { /// flag. #[inline] pub unsafe fn cast_to_usize(self) -> usize { - // SAFETY: this ok because of the internal represention of Arc, which - // is a pointer and phantom data (so the size of a pointer -> a usize). - unsafe { mem::transmute(self.inner) } + Arc::into_raw(self.inner) as usize } /// Converts from an unsafe usize value. Useful for retrieving a pipe's state @@ -58,7 +55,10 @@ impl SignalToken { pub unsafe fn cast_from_usize(signal_ptr: usize) -> SignalToken { // SAFETY: this ok because of the internal represention of Arc, which // is a pointer and phantom data (so the size of a pointer -> a usize). - SignalToken { inner: unsafe { mem::transmute(signal_ptr) } } + // + // NOTE: The call to `from_raw` is used in conjuction with the call to + // `into_raw` made in the `SignalToken::cast_to_usize` method. + SignalToken { inner: unsafe { Arc::from_raw(signal_ptr as *const _) } } } } diff --git a/src/libstd/sync/mpsc/mod.rs b/src/libstd/sync/mpsc/mod.rs index 3a9e62f3ef99c..65c615b22c2b6 100644 --- a/src/libstd/sync/mpsc/mod.rs +++ b/src/libstd/sync/mpsc/mod.rs @@ -651,7 +651,7 @@ trait UnsafeFlavor { // SAFETY: We are sure the inner value will never be NUL when this is // called, the invariants of the module make it so. // - // It is also ensured that no other (mutable) reference will be handed + // The caller ensures that no other (mutable) reference will be handed // out while the one returned here is in action. unsafe { &mut *self.inner_unsafe().get() } } diff --git a/src/libstd/sync/mpsc/spsc_queue.rs b/src/libstd/sync/mpsc/spsc_queue.rs index 19dea7f24d179..8d8e185cbaf6b 100644 --- a/src/libstd/sync/mpsc/spsc_queue.rs +++ b/src/libstd/sync/mpsc/spsc_queue.rs @@ -99,8 +99,8 @@ impl Queue Self { let n1 = Node::new(); let n2 = Node::new(); - // SAFETY: At this point we know the pointer is not NUL since it was - // build just above. + // SAFETY: At this point we know the pointer is valid: it was built + // just above. unsafe { (*n1).next.store(n2, Ordering::Relaxed) } Queue { consumer: CacheAligned::new(Consumer { @@ -135,13 +135,10 @@ impl Queue *mut Node { - // First try to see if we can consume the 'first' node for our uses. - // SAFEY: Since `self` is a valid reference, accessing its inner values - // can be considered safe (without checking for NUL pointers). - // - // Dereferencing of `ret` are safe too because the pointer comes from - // `self` once again. + // SAFEY: ??? Explain why the pointers are safe to dereference and why + // returning a mutable pointer is ok. ??? unsafe { + // First try to see if we can consume the 'first' node for our uses. if *self.producer.first.get() != *self.producer.tail_copy.get() { let ret = *self.producer.first.get(); *self.producer.0.first.get() = (*ret).next.load(Ordering::Relaxed); @@ -322,7 +319,7 @@ mod tests { } unsafe fn stress_bound(bound: usize) { - let q = Arc::new(Queue::with_additions(bound, (), ())); + let q = Arc::new(unsafe { Queue::with_additions(bound, (), ()) }); let (tx, rx) = channel(); let q2 = q.clone();