From e7c8fbf4573268e0fb91fa46376d82d505683262 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 23 Jul 2022 01:54:46 +0900 Subject: [PATCH] epoch: Fix stacked borrows violations --- ci/miri.sh | 14 +++----- crossbeam-epoch/build.rs | 3 ++ crossbeam-epoch/src/atomic.rs | 56 +++++++++++++++++++++----------- crossbeam-epoch/src/internal.rs | 20 ++++++------ crossbeam-epoch/src/sync/list.rs | 32 +++++++++--------- 5 files changed, 70 insertions(+), 55 deletions(-) diff --git a/ci/miri.sh b/ci/miri.sh index 1e9dde6f4..e3f380b42 100755 --- a/ci/miri.sh +++ b/ci/miri.sh @@ -12,33 +12,27 @@ export RUSTFLAGS="${RUSTFLAGS:-} -Z randomize-layout" MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation" \ cargo miri test \ -p crossbeam-queue \ - -p crossbeam-utils 2>&1 | ts -i '%.s ' + -p crossbeam-utils \ + -p crossbeam-epoch 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-disable-isolation -Zmiri-ignore-leaks" \ cargo miri test \ -p crossbeam-channel 2>&1 | ts -i '%.s ' -# -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545 -MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-disable-stacked-borrows" \ - cargo miri test \ - -p crossbeam-epoch 2>&1 | ts -i '%.s ' - # -Zmiri-ignore-leaks is needed for https://github.com/crossbeam-rs/crossbeam/issues/614 # -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545 MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks" \ cargo miri test \ -p crossbeam-skiplist 2>&1 | ts -i '%.s ' -# -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545 # -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-disable-stacked-borrows -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ +MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \ cargo miri test \ -p crossbeam-deque 2>&1 | ts -i '%.s ' -# -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545 -MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-stacked-borrows" \ +MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check" \ cargo miri test \ -p crossbeam 2>&1 | ts -i '%.s ' diff --git a/crossbeam-epoch/build.rs b/crossbeam-epoch/build.rs index b3bd15a5a..2a7563509 100644 --- a/crossbeam-epoch/build.rs +++ b/crossbeam-epoch/build.rs @@ -48,6 +48,9 @@ fn main() { println!("cargo:rustc-cfg=crossbeam_no_atomic_cas"); } + if !cfg.probe_rustc_version(1, 51) { + println!("cargo:rustc-cfg=crossbeam_no_raw_ref_macros"); + } if !cfg.probe_rustc_version(1, 61) { println!("cargo:rustc-cfg=crossbeam_no_const_fn_trait_bound"); } diff --git a/crossbeam-epoch/src/atomic.rs b/crossbeam-epoch/src/atomic.rs index aeacca356..103c7a209 100644 --- a/crossbeam-epoch/src/atomic.rs +++ b/crossbeam-epoch/src/atomic.rs @@ -183,8 +183,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. /// @@ -192,9 +192,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. /// @@ -202,7 +202,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 ()); } @@ -216,12 +216,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 ()) { @@ -276,12 +276,16 @@ 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::>(); + #[cfg(not(crossbeam_no_raw_ref_macros))] + let ptr = ptr::addr_of!((*array).elements) as *const _; + #[cfg(crossbeam_no_raw_ref_macros)] + let ptr = (*array).elements.as_ptr(); + slice::from_raw_parts(ptr, (*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::>(); slice::from_raw_parts_mut(array.elements.as_mut_ptr(), array.len) } @@ -1013,7 +1017,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) } } @@ -1299,14 +1303,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) } } } @@ -1492,7 +1496,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// ``` pub unsafe fn deref(&self) -> &'g T { let (raw, _) = decompose_tag::(self.data); - T::deref(raw) + &*T::as_ptr(raw) } /// Dereferences the pointer. @@ -1534,7 +1538,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { /// ``` pub unsafe fn deref_mut(&mut self) -> &'g mut T { let (raw, _) = decompose_tag::(self.data); - T::deref_mut(raw) + &mut *T::as_mut_ptr(raw) } /// Converts the pointer to a reference. @@ -1574,10 +1578,24 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> { if raw.is_null() { None } else { - Some(T::deref(raw)) + Some(&*T::as_ptr(raw)) } } + pub(crate) unsafe fn as_non_null(&self) -> Option<*const T> { + let (raw, _) = decompose_tag::(self.data); + if raw.is_null() { + None + } else { + Some(T::as_ptr(raw)) + } + } + + pub(crate) unsafe fn as_ptr(&self) -> *const T { + let (raw, _) = decompose_tag::(self.data); + T::as_ptr(raw) + } + /// Takes ownership of the pointee. /// /// # Panics diff --git a/crossbeam-epoch/src/internal.rs b/crossbeam-epoch/src/internal.rs index 207ee19f6..1d54e9899 100644 --- a/crossbeam-epoch/src/internal.rs +++ b/crossbeam-epoch/src/internal.rs @@ -534,24 +534,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..b34593248 100644 --- a/crossbeam-epoch/src/sync/list.rs +++ b/crossbeam-epoch/src/sync/list.rs @@ -66,7 +66,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 +80,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 +88,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 +173,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 +225,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 +236,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 { self.curr.as_non_null() } { + let succ = unsafe { (*c).next.load(Acquire, self.guard) }; if succ.tag() == 1 { // This entry was removed. Try unlinking it from the list. @@ -257,7 +257,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 +284,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).next }; self.curr = succ; - return Some(Ok(unsafe { C::element_of(c) })); + return Some(Ok(unsafe { &*C::element_of(c) })); } // We reached the end of the list. @@ -303,16 +303,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))); } }