Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

epoch: Fix stacked borrows violations #871

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions ci/miri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,25 +14,24 @@ MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disab
MIRI_LEAK_CHECK='1' \
cargo miri test \
-p crossbeam-channel \
-p crossbeam-epoch \
-p crossbeam-queue \
-p crossbeam-utils 2>&1 | ts -i '%.s '
-p crossbeam-utils \
-p crossbeam 2>&1 | ts -i '%.s '

# -Zmiri-ignore-leaks is needed because we use detached threads in tests in tests/golang.rs: 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 --test golang 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
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-tree-borrows" \
cargo miri test \
-p crossbeam-epoch \
-p crossbeam-skiplist \
-p crossbeam 2>&1 | ts -i '%.s '
-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-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-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 '
64 changes: 39 additions & 25 deletions crossbeam-epoch/src/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use core::marker::PhantomData;
use core::mem::{self, MaybeUninit};
use core::ops::{Deref, DerefMut};
use core::ptr;
use core::slice;

use crate::guard::Guard;
#[cfg(not(miri))]
Expand Down Expand Up @@ -125,26 +124,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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer a dereference, but I'm not sure what is a good API name and description.

///
/// # 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 @@ -158,12 +157,12 @@ impl<T> Pointable for T {
Box::into_raw(Box::new(init)).cast::<()>()
}

unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self {
unsafe { &*(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 {
unsafe { &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 @@ -225,17 +224,22 @@ impl<T> Pointable for [MaybeUninit<T>] {
}
}

unsafe fn deref<'a>(ptr: *mut ()) -> &'a Self {
unsafe fn as_ptr(ptr: *mut ()) -> *const Self {
unsafe {
let array = &*(ptr as *const Array<T>);
slice::from_raw_parts(array.elements.as_ptr(), array.len)
let len = (*ptr.cast::<Array<T>>()).len;
// Use addr_of_mut for stacked borrows: https://github.com/rust-lang/miri/issues/1976
let elements =
ptr::addr_of_mut!((*ptr.cast::<Array<T>>()).elements).cast::<MaybeUninit<T>>();
ptr::slice_from_raw_parts(elements, len)
}
}

unsafe fn deref_mut<'a>(ptr: *mut ()) -> &'a mut Self {
unsafe fn as_mut_ptr(ptr: *mut ()) -> *mut Self {
unsafe {
let array = &mut *ptr.cast::<Array<T>>();
slice::from_raw_parts_mut(array.elements.as_mut_ptr(), array.len)
let len = (*ptr.cast::<Array<T>>()).len;
let elements =
ptr::addr_of_mut!((*ptr.cast::<Array<T>>()).elements).cast::<MaybeUninit<T>>();
ptr::slice_from_raw_parts_mut(elements, len)
}
}

Expand Down Expand Up @@ -846,7 +850,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 @@ -1134,14 +1138,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 @@ -1291,6 +1295,18 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> {
raw.is_null()
}

#[doc(hidden)]
pub unsafe fn as_ptr(&self) -> *const T {
let (raw, _) = decompose_tag::<T>(self.data);
unsafe { T::as_ptr(raw) }
}

#[doc(hidden)]
pub unsafe fn as_mut_ptr(&self) -> *mut T {
let (raw, _) = decompose_tag::<T>(self.data);
unsafe { T::as_mut_ptr(raw) }
}

/// Dereferences the pointer.
///
/// Returns a reference to the pointee that is valid during the lifetime `'g`.
Expand Down Expand Up @@ -1324,8 +1340,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::<T>(self.data);
unsafe { T::deref(raw) }
unsafe { &*self.as_ptr() }
}

/// Dereferences the pointer.
Expand Down Expand Up @@ -1366,8 +1381,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::<T>(self.data);
unsafe { T::deref_mut(raw) }
unsafe { &mut *self.as_mut_ptr() }
}

/// Converts the pointer to a reference.
Expand Down Expand Up @@ -1407,7 +1421,7 @@ impl<'g, T: ?Sized + Pointable> Shared<'g, T> {
if raw.is_null() {
None
} else {
Some(unsafe { T::deref(raw) })
Some(unsafe { &*T::as_ptr(raw) })
}
}

Expand Down Expand Up @@ -1569,7 +1583,7 @@ impl<T: ?Sized + Pointable> fmt::Debug for Shared<'_, T> {

impl<T: ?Sized + Pointable> 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)
}
}

Expand Down
26 changes: 12 additions & 14 deletions crossbeam-epoch/src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,9 @@ pub(crate) struct Local {
pin_count: Cell<Wrapping<usize>>,

/// The local epoch.
epoch: CachePadded<AtomicEpoch>,
// TODO
// epoch: CachePadded<AtomicEpoch>,
epoch: AtomicEpoch,
}

// Make sure `Local` is less than or equal to 2048 bytes.
Expand Down Expand Up @@ -324,7 +326,9 @@ impl Local {
guard_count: Cell::new(0),
handle_count: Cell::new(1),
pin_count: Cell::new(Wrapping(0)),
epoch: CachePadded::new(AtomicEpoch::new(Epoch::starting())),
// TODO
// epoch: CachePadded::new(AtomicEpoch::new(Epoch::starting())),
epoch: AtomicEpoch::new(Epoch::starting()),
})
.into_shared(unprotected());
collector.global.locals.insert(local, unprotected());
Expand Down Expand Up @@ -535,24 +539,18 @@ impl Local {
}

impl IsElement<Self> for Local {
fn entry_of(local: &Self) -> &Entry {
fn entry_of(local: *const Self) -> *const Entry {
// SAFETY: `Local` is `repr(C)` and `entry` is the first field of it.
unsafe {
let entry_ptr = (local as *const Self).cast::<Entry>();
&*entry_ptr
}
local.cast::<Entry>()
}

unsafe fn element_of(entry: &Entry) -> &Self {
unsafe fn element_of(entry: *const Entry) -> *const Self {
// SAFETY: `Local` is `repr(C)` and `entry` is the first field of it.
unsafe {
let local_ptr = (entry as *const Entry).cast::<Self>();
&*local_ptr
}
entry.cast::<Self>()
}

unsafe fn finalize(entry: &Entry, guard: &Guard) {
unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)) }
unsafe fn finalize(entry: *const Entry, guard: &Guard) {
unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry))) }
}
}

Expand Down
37 changes: 19 additions & 18 deletions crossbeam-epoch/src/sync/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! 2002. <http://dl.acm.org/citation.cfm?id=564870.564881>

use core::marker::PhantomData;
use core::ptr::NonNull;
use core::sync::atomic::Ordering::{Acquire, Relaxed, Release};

use crate::{unprotected, Atomic, Guard, Shared};
Expand Down Expand Up @@ -66,7 +67,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 +81,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 +174,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(unsafe { container.deref() });
let entry: *const Entry = C::entry_of(unsafe { 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);
unsafe { (*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 @@ -220,12 +221,12 @@ impl<T, C: IsElement<T>> Drop for List<T, C> {
unsafe {
let guard = unprotected();
let mut curr = self.head.load(Relaxed, guard);
while let Some(c) = curr.as_ref() {
let succ = c.next.load(Relaxed, guard);
while let Some(c) = NonNull::new(curr.as_ptr() as *mut Entry) {
let succ = c.as_ref().next.load(Relaxed, guard);
// 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 +237,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 { 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.
Expand All @@ -257,7 +258,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 +285,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.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.
Expand All @@ -303,16 +304,16 @@ mod tests {
use std::sync::Barrier;

impl IsElement<Self> for Entry {
fn entry_of(entry: &Self) -> &Entry {
fn entry_of(entry: *const Self) -> *const Entry {
entry
}

unsafe fn element_of(entry: &Entry) -> &Self {
unsafe fn element_of(entry: *const Entry) -> *const Self {
entry
}

unsafe fn finalize(entry: &Entry, guard: &Guard) {
unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry) as *const _)) }
unsafe fn finalize(entry: *const Entry, guard: &Guard) {
unsafe { guard.defer_destroy(Shared::from(Self::element_of(entry))) }
}
}

Expand Down