Skip to content

Commit

Permalink
epoch: Fix stacked borrows violations
Browse files Browse the repository at this point in the history
  • Loading branch information
taiki-e committed Jul 23, 2022
1 parent 0fbc9e2 commit 525ae4f
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 55 deletions.
14 changes: 4 additions & 10 deletions ci/miri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 '
3 changes: 3 additions & 0 deletions crossbeam-epoch/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
58 changes: 39 additions & 19 deletions crossbeam-epoch/src/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,26 +183,26 @@ 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.
///
/// # Safety
///
/// - 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.
///
/// # Safety
///
/// - 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 ());
}
Expand All @@ -216,12 +216,12 @@ impl<T> 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::<T>()
unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self {
ptr.cast::<T>()
}

unsafe fn drop(ptr: *mut ()) {
Expand Down Expand Up @@ -276,13 +276,19 @@ impl<T> Pointable for [MaybeUninit<T>] {
ptr.cast::<()>()
}

unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self {
let array = &*(ptr as *const Array<T>);
slice::from_raw_parts(array.elements.as_ptr(), array.len)
unsafe fn as_ptr(ptr: *mut ()) -> *const Self {
let array = ptr.cast::<Array<T>>();
#[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();
// TODO: use ptr::slice_from_raw_parts once we bump MSRV to 1.42
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::<Array<T>>();
// 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)
}

Expand Down Expand Up @@ -1013,7 +1019,7 @@ impl<T: ?Sized + Pointable> fmt::Pointer for Atomic<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let data = self.data.load(Ordering::SeqCst);
let (raw, _) = decompose_tag::<T>(data);
fmt::Pointer::fmt(&(unsafe { T::deref(raw) as *const _ }), f)
fmt::Pointer::fmt(&(unsafe { T::as_ptr(raw) }), f)
}
}

Expand Down Expand Up @@ -1299,14 +1305,14 @@ impl<T: ?Sized + Pointable> Deref for Owned<T> {

fn deref(&self) -> &T {
let (raw, _) = decompose_tag::<T>(self.data);
unsafe { T::deref(raw) }
unsafe { &*T::as_ptr(raw) }
}
}

impl<T: ?Sized + Pointable> DerefMut for Owned<T> {
fn deref_mut(&mut self) -> &mut T {
let (raw, _) = decompose_tag::<T>(self.data);
unsafe { T::deref_mut(raw) }
unsafe { &mut *T::as_mut_ptr(raw) }
}
}

Expand Down Expand Up @@ -1492,7 +1498,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> {
/// ```
pub unsafe fn deref(&self) -> &'g T {
let (raw, _) = decompose_tag::<T>(self.data);
T::deref(raw)
&*T::as_ptr(raw)
}

/// Dereferences the pointer.
Expand Down Expand Up @@ -1534,7 +1540,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> {
/// ```
pub unsafe fn deref_mut(&mut self) -> &'g mut T {
let (raw, _) = decompose_tag::<T>(self.data);
T::deref_mut(raw)
&mut *T::as_mut_ptr(raw)
}

/// Converts the pointer to a reference.
Expand Down Expand Up @@ -1574,10 +1580,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::<T>(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::<T>(self.data);
T::as_ptr(raw)
}

/// Takes ownership of the pointee.
///
/// # Panics
Expand Down
20 changes: 10 additions & 10 deletions crossbeam-epoch/src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,24 +536,24 @@ impl Local {
}

impl IsElement<Local> 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::<u8>()
.add(offset_of!(Local, entry))
.cast::<Entry>();
&*entry_ptr
.cast::<Entry>()
}
}

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::<u8>()
.sub(offset_of!(Local, entry))
.cast::<Local>();
&*local_ptr
.cast::<Local>()
}

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)));
}
}

Expand Down
32 changes: 16 additions & 16 deletions crossbeam-epoch/src/sync/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub(crate) struct Entry {
///
pub(crate) trait IsElement<T> {
/// 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.
///
Expand All @@ -80,15 +80,15 @@ pub(crate) trait IsElement<T> {
///
/// 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.
///
/// # Safety
///
/// 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`.
Expand Down Expand Up @@ -173,16 +173,16 @@ impl<T, C: IsElement<T>> List<T, C> {
// 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
Expand Down Expand Up @@ -225,7 +225,7 @@ impl<T, C: IsElement<T>> Drop for List<T, C> {
// 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;
}
}
Expand All @@ -236,8 +236,8 @@ impl<'g, T: 'g, C: IsElement<T>> Iterator for Iter<'g, T, C> {
type Item = Result<&'g T, IterError>;

fn next(&mut self) -> Option<Self::Item> {
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.
Expand All @@ -257,7 +257,7 @@ impl<'g, T: 'g, C: IsElement<T>> 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`.
Expand All @@ -284,10 +284,10 @@ impl<'g, T: 'g, C: IsElement<T>> 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.
Expand All @@ -303,16 +303,16 @@ mod tests {
use std::sync::Barrier;

impl IsElement<Entry> 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)));
}
}

Expand Down

0 comments on commit 525ae4f

Please sign in to comment.