From a3fec7d95fcc59e827082cee3edca4f5ed82ff70 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sun, 20 Nov 2022 17:11:27 +0900 Subject: [PATCH] epoch: Fix stacked borrows violations --- ci/miri.sh | 15 +++----- crossbeam-epoch/src/atomic.rs | 60 ++++++++++++++++++++------------ crossbeam-epoch/src/internal.rs | 20 +++++------ crossbeam-epoch/src/sync/list.rs | 33 +++++++++--------- 4 files changed, 70 insertions(+), 58 deletions(-) diff --git a/ci/miri.sh b/ci/miri.sh index edb71e9d1..429f026d6 100755 --- a/ci/miri.sh +++ b/ci/miri.sh @@ -12,29 +12,24 @@ export RUSTFLAGS="${RUSTFLAGS:-} -Z randomize-layout" MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation" \ cargo miri test \ -p crossbeam-queue \ - -p crossbeam-utils 2>&1 | ts -i '%.s ' + -p crossbeam-utils \ + -p crossbeam-epoch \ + -p crossbeam 2>&1 | ts -i '%.s ' # -Zmiri-ignore-leaks is needed because we use detached threads in tests/docs: https://github.com/rust-lang/miri/issues/1371 MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation -Zmiri-ignore-leaks" \ cargo miri test \ -p crossbeam-channel 2>&1 | ts -i '%.s ' -# Use Tree Borrows instead of Stacked Borrows because epoch is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/545#issuecomment-1192785003 -MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation -Zmiri-tree-borrows" \ - cargo miri test \ - -p crossbeam-epoch \ - -p crossbeam 2>&1 | ts -i '%.s ' - -# Use Tree Borrows instead of Stacked Borrows because epoch is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/545#issuecomment-1192785003 +# Use Tree Borrows instead of Stacked Borrows because skiplist is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/878 # -Zmiri-ignore-leaks is needed for https://github.com/crossbeam-rs/crossbeam/issues/614 MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation -Zmiri-tree-borrows -Zmiri-ignore-leaks" \ cargo miri test \ -p crossbeam-skiplist 2>&1 | ts -i '%.s ' -# Use Tree Borrows instead of Stacked Borrows because epoch is not compatible with Stacked Borrows: https://github.com/crossbeam-rs/crossbeam/issues/545#issuecomment-1192785003 # -Zmiri-compare-exchange-weak-failure-rate=0.0 is needed because some sequential tests (e.g., # doctest of Stealer::steal) incorrectly assume that sequential weak CAS will never fail. # -Zmiri-preemption-rate=0 is needed because this code technically has UB and Miri catches that. -MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation -Zmiri-tree-borrows -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ +MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-retag-fields -Zmiri-disable-isolation -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ cargo miri test \ -p crossbeam-deque 2>&1 | ts -i '%.s ' diff --git a/crossbeam-epoch/src/atomic.rs b/crossbeam-epoch/src/atomic.rs index b33a8ac86..7fbacc18d 100644 --- a/crossbeam-epoch/src/atomic.rs +++ b/crossbeam-epoch/src/atomic.rs @@ -14,7 +14,9 @@ use crate::guard::Guard; use crate::primitive::sync::atomic::AtomicPtr; #[cfg(not(miri))] use crate::primitive::sync::atomic::AtomicUsize; + use crossbeam_utils::atomic::AtomicConsume; +use memoffset::offset_of; /// The error returned on failed compare-and-swap operation. pub struct CompareExchangeError<'g, T: ?Sized + Pointable, P: Pointer> { @@ -113,8 +115,8 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be mutably dereferenced by [`Pointable::deref_mut`] concurrently. - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self; + /// - `ptr` should not be mutably dereferenced by [`Pointable::as_mut_ptr`] concurrently. + unsafe fn as_ptr(ptr: *mut ()) -> *const Self; /// Mutably dereferences the given pointer. /// @@ -122,9 +124,9 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be dereferenced by [`Pointable::deref`] or [`Pointable::deref_mut`] + /// - `ptr` should not be dereferenced by [`Pointable::as_ptr`] or [`Pointable::as_mut_ptr`] /// concurrently. - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self; + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self; /// Drops the object pointed to by the given pointer. /// @@ -132,7 +134,7 @@ pub trait Pointable { /// /// - The given `ptr` should have been initialized with [`Pointable::init`]. /// - `ptr` should not have yet been dropped by [`Pointable::drop`]. - /// - `ptr` should not be dereferenced by [`Pointable::deref`] or [`Pointable::deref_mut`] + /// - `ptr` should not be dereferenced by [`Pointable::as_ptr`] or [`Pointable::as_mut_ptr`] /// concurrently. unsafe fn drop(ptr: *mut ()); } @@ -146,12 +148,12 @@ impl Pointable for T { Box::into_raw(Box::new(init)).cast::<()>() } - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self { - &*(ptr as *const T) + unsafe fn as_ptr(ptr: *mut ()) -> *const Self { + ptr as *const T } - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self { - &mut *ptr.cast::() + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self { + ptr.cast::() } unsafe fn drop(ptr: *mut ()) { @@ -206,13 +208,19 @@ impl Pointable for [MaybeUninit] { ptr.cast::<()>() } - unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self { - let array = &*(ptr as *const Array); - slice::from_raw_parts(array.elements.as_ptr(), array.len) + unsafe fn as_ptr(ptr: *mut ()) -> *const Self { + let array = ptr.cast::>(); + let elements = array + .cast::() + .add(offset_of!(Array, elements)) + .cast::>(); + // TODO: use ptr::slice_from_raw_parts once we bump MSRV to 1.42 + slice::from_raw_parts(elements, (*array).len) } - unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self { + unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self { let array = &mut *ptr.cast::>(); + // TODO: use ptr::slice_from_raw_parts_mut once we bump MSRV to 1.42 slice::from_raw_parts_mut(array.elements.as_mut_ptr(), array.len) } @@ -808,7 +816,7 @@ impl fmt::Pointer for Atomic { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let data = self.data.load(Ordering::SeqCst); let (raw, _) = decompose_tag::(data); - fmt::Pointer::fmt(&(unsafe { T::deref(raw) as *const _ }), f) + fmt::Pointer::fmt(&(unsafe { T::as_ptr(raw) }), f) } } @@ -1096,14 +1104,14 @@ impl Deref for Owned { fn deref(&self) -> &T { let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref(raw) } + unsafe { &*T::as_ptr(raw) } } } impl DerefMut for Owned { fn deref_mut(&mut self) -> &mut T { let (raw, _) = decompose_tag::(self.data); - unsafe { T::deref_mut(raw) } + unsafe { &mut *T::as_mut_ptr(raw) } } } @@ -1256,6 +1264,16 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { raw.is_null() } + pub(crate) unsafe fn as_ptr(&self) -> *const T { + let (raw, _) = decompose_tag::(self.data); + T::as_ptr(raw) + } + + pub(crate) unsafe fn as_mut_ptr(&self) -> *mut T { + let (raw, _) = decompose_tag::(self.data); + T::as_mut_ptr(raw) + } + /// Dereferences the pointer. /// /// Returns a reference to the pointee that is valid during the lifetime `'g`. @@ -1289,8 +1307,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// # unsafe { drop(a.into_owned()); } // avoid leak /// ``` pub unsafe fn deref(&self) -> &'g T { - let (raw, _) = decompose_tag::(self.data); - T::deref(raw) + &*self.as_ptr() } /// Dereferences the pointer. @@ -1331,8 +1348,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// # unsafe { drop(a.into_owned()); } // avoid leak /// ``` pub unsafe fn deref_mut(&mut self) -> &'g mut T { - let (raw, _) = decompose_tag::(self.data); - T::deref_mut(raw) + &mut *self.as_mut_ptr() } /// Converts the pointer to a reference. @@ -1372,7 +1388,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { if raw.is_null() { None } else { - Some(T::deref(raw)) + Some(&*T::as_ptr(raw)) } } @@ -1534,7 +1550,7 @@ impl fmt::Debug for Shared<'_, T> { impl fmt::Pointer for Shared<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Pointer::fmt(&(unsafe { self.deref() as *const _ }), f) + fmt::Pointer::fmt(&(unsafe { self.as_ptr() }), f) } } diff --git a/crossbeam-epoch/src/internal.rs b/crossbeam-epoch/src/internal.rs index 486993c63..6d2f464ea 100644 --- a/crossbeam-epoch/src/internal.rs +++ b/crossbeam-epoch/src/internal.rs @@ -536,24 +536,24 @@ impl Local { } impl IsElement for Local { - fn entry_of(local: &Local) -> &Entry { + fn entry_of(local: *const Local) -> *const Entry { unsafe { - let entry_ptr = (local as *const Local as *const u8) + local + .cast::() .add(offset_of!(Local, entry)) - .cast::(); - &*entry_ptr + .cast::() } } - unsafe fn element_of(entry: &Entry) -> &Local { - let local_ptr = (entry as *const Entry as *const u8) + unsafe fn element_of(entry: *const Entry) -> *const Local { + entry + .cast::() .sub(offset_of!(Local, entry)) - .cast::(); - &*local_ptr + .cast::() } - unsafe fn finalize(entry: &Entry, guard: &Guard) { - guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)); + unsafe fn finalize(entry: *const Entry, guard: &Guard) { + guard.defer_destroy(Shared::from(Self::element_of(entry))); } } diff --git a/crossbeam-epoch/src/sync/list.rs b/crossbeam-epoch/src/sync/list.rs index 52ffd6fca..f4f1d61e4 100644 --- a/crossbeam-epoch/src/sync/list.rs +++ b/crossbeam-epoch/src/sync/list.rs @@ -4,6 +4,7 @@ //! 2002. use core::marker::PhantomData; +use core::ptr::NonNull; use core::sync::atomic::Ordering::{Acquire, Relaxed, Release}; use crate::{unprotected, Atomic, Guard, Shared}; @@ -66,7 +67,7 @@ pub(crate) struct Entry { /// pub(crate) trait IsElement { /// Returns a reference to this element's `Entry`. - fn entry_of(_: &T) -> &Entry; + fn entry_of(_: *const T) -> *const Entry; /// Given a reference to an element's entry, returns that element. /// @@ -80,7 +81,7 @@ pub(crate) trait IsElement { /// /// The caller has to guarantee that the `Entry` is called with was retrieved from an instance /// of the element type (`T`). - unsafe fn element_of(_: &Entry) -> &T; + unsafe fn element_of(_: *const Entry) -> *const T; /// The function that is called when an entry is unlinked from list. /// @@ -88,7 +89,7 @@ pub(crate) trait IsElement { /// /// The caller has to guarantee that the `Entry` is called with was retrieved from an instance /// of the element type (`T`). - unsafe fn finalize(_: &Entry, _: &Guard); + unsafe fn finalize(_: *const Entry, _: &Guard); } /// A lock-free, intrusive linked list of type `T`. @@ -173,16 +174,16 @@ impl> List { // Insert right after head, i.e. at the beginning of the list. let to = &self.head; // Get the intrusively stored Entry of the new element to insert. - let entry: &Entry = C::entry_of(container.deref()); + let entry: *const Entry = C::entry_of(container.as_ptr()); // Make a Shared ptr to that Entry. - let entry_ptr = Shared::from(entry as *const _); + let entry_ptr = Shared::from(entry); // Read the current successor of where we want to insert. let mut next = to.load(Relaxed, guard); loop { // Set the Entry of the to-be-inserted element to point to the previous successor of // `to`. - entry.next.store(next, Relaxed); + (*entry).next.store(next, Relaxed); match to.compare_exchange_weak(next, entry_ptr, Release, Relaxed, guard) { Ok(_) => break, // We lost the race or weak CAS failed spuriously. Update the successor and try @@ -225,7 +226,7 @@ impl> Drop for List { // Verify that all elements have been removed from the list. assert_eq!(succ.tag(), 1); - C::finalize(curr.deref(), guard); + C::finalize(curr.as_ptr(), guard); curr = succ; } } @@ -236,8 +237,8 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { type Item = Result<&'g T, IterError>; fn next(&mut self) -> Option { - while let Some(c) = unsafe { self.curr.as_ref() } { - let succ = c.next.load(Acquire, self.guard); + while let Some(c) = unsafe { NonNull::new(self.curr.as_ptr() as *mut Entry) } { + let succ = unsafe { c.as_ref().next.load(Acquire, self.guard) }; if succ.tag() == 1 { // This entry was removed. Try unlinking it from the list. @@ -257,7 +258,7 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { // deallocation. Deferred drop is okay, because `list.delete()` can only be // called if `T: 'static`. unsafe { - C::finalize(self.curr.deref(), self.guard); + C::finalize(self.curr.as_ptr(), self.guard); } // `succ` is the new value of `self.pred`. @@ -284,10 +285,10 @@ impl<'g, T: 'g, C: IsElement> Iterator for Iter<'g, T, C> { } // Move one step forward. - self.pred = &c.next; + self.pred = unsafe { &(*c.as_ptr()).next }; self.curr = succ; - return Some(Ok(unsafe { C::element_of(c) })); + return Some(Ok(unsafe { &*C::element_of(c.as_ptr()) })); } // We reached the end of the list. @@ -303,16 +304,16 @@ mod tests { use std::sync::Barrier; impl IsElement for Entry { - fn entry_of(entry: &Entry) -> &Entry { + fn entry_of(entry: *const Entry) -> *const Entry { entry } - unsafe fn element_of(entry: &Entry) -> &Entry { + unsafe fn element_of(entry: *const Entry) -> *const Entry { entry } - unsafe fn finalize(entry: &Entry, guard: &Guard) { - guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)); + unsafe fn finalize(entry: *const Entry, guard: &Guard) { + guard.defer_destroy(Shared::from(Self::element_of(entry))); } }