Revise the safety conditions for OwnerPtr

Co-authored-by: Zhang Junyang <junyang@stu.pku.edu.cn>

ostd/src/sync/rcu/mod.rs

@@ -124,7 +124,7 @@ unsafe impl<P: OwnerPtr, const NULLABLE: bool> Sync for Rcu<P, NULLABLE> where
 impl<P: OwnerPtr> Rcu<P, false> {
     /// Creates a new RCU cell with the given pointer.
     pub fn new(pointer: P) -> Self {
-        let ptr = <P as OwnerPtr>::into_raw(pointer).cast_mut();
+        let ptr = <P as OwnerPtr>::into_raw(pointer).as_ptr();
         let ptr = AtomicPtr::new(ptr);
         Self {
             ptr,
@@ -162,7 +162,7 @@ impl<P: OwnerPtr + Send, const NULLABLE: bool> Rcu<P, NULLABLE> {
     /// synchronized writes with locks. Otherwise, you can use [`Self::read`]
     /// and then [`RcuReadGuard::compare_exchange`] to update the pointer.
     pub fn update(&self, new_ptr: P) {
-        let new_ptr = <P as OwnerPtr>::into_raw(new_ptr).cast_mut();
+        let new_ptr = <P as OwnerPtr>::into_raw(new_ptr).as_ptr();
         let old_raw_ptr = self.ptr.swap(new_ptr, AcqRel);
 
         if let Some(p) = NonNull::new(old_raw_ptr) {
@@ -245,15 +245,18 @@ impl<P: OwnerPtr + Send, const NULLABLE: bool> RcuReadGuard<'_, P, NULLABLE> {
     /// This API does not help to avoid
     /// [the ABA problem](https://en.wikipedia.org/wiki/ABA_problem).
     pub fn compare_exchange(self, new_ptr: P) -> Result<(), P> {
-        let new_ptr = <P as OwnerPtr>::into_raw(new_ptr).cast_mut();
+        let new_ptr = <P as OwnerPtr>::into_raw(new_ptr);
 
         if self
             .rcu
             .ptr
-            .compare_exchange(self.obj_ptr, new_ptr, AcqRel, Acquire)
+            .compare_exchange(self.obj_ptr, new_ptr.as_ptr(), AcqRel, Acquire)
             .is_err()
         {
-            // SAFETY: It was previously returned by `into_raw`.
+            // SAFETY:
+            // 1. It was previously returned by `into_raw`.
+            // 2. The `compare_exchange` fails so the pointer will not
+            //    be used anymore.
             return Err(unsafe { <P as OwnerPtr>::from_raw(new_ptr) });
         }
 
@@ -271,10 +274,25 @@ impl<P: OwnerPtr + Send, const NULLABLE: bool> RcuReadGuard<'_, P, NULLABLE> {
 /// The pointer must be previously returned by `into_raw` and the pointer
 /// must be only be dropped once.
 unsafe fn delay_drop<P: OwnerPtr + Send>(pointer: NonNull<<P as OwnerPtr>::Target>) {
-    // SAFETY: The pointer is not NULL.
+    struct ForceSend<P: OwnerPtr>(NonNull<<P as OwnerPtr>::Target>);
-    let p = unsafe { <P as OwnerPtr>::from_raw(pointer.as_ptr().cast_const()) };
+    // SAFETY: Sending a raw pointer to another task is safe as long as
+    // the pointer access in another task is safe (guaranteed by the trait
+    // bound `P: Send`).
+    unsafe impl<P: OwnerPtr + Send> Send for ForceSend<P> {}
+
+    let pointer: ForceSend<P> = ForceSend(pointer);
+
     let rcu_monitor = RCU_MONITOR.get().unwrap();
     rcu_monitor.after_grace_period(move || {
+        // This is necessary to make the Rust compiler to move the entire
+        // `ForceSend` structure into the closure.
+        let pointer = pointer;
+
+        // SAFETY:
+        // 1. The pointer was previously returned by `into_raw`.
+        // 2. The pointer won't be used anymore since the grace period has
+        //    finished and this is the only time the pointer gets dropped.
+        let p = unsafe { <P as OwnerPtr>::from_raw(pointer.0) };
         drop(p);
     });
 }

ostd/src/sync/rcu/owner_ptr.rs

@@ -1,14 +1,23 @@
 // SPDX-License-Identifier: MPL-2.0
 
+use core::ptr::NonNull;
+
 use crate::prelude::*;
 
 /// A trait that abstracts pointers that have the ownership of the objects they
 /// refer to.
 ///
-/// The most typical examples smart pointer types like `Box<T>` and `Arc<T>`.
+/// The most typical examples smart pointer types like `Box<T>` and `Arc<T>`,
-///
 /// which can be converted to and from the raw pointer type of `*const T`.
-pub trait OwnerPtr: 'static {
+///
+/// # Safety
+///
+/// This trait must be implemented correctly (according to the doc comments for
+/// each method). Types like [`Rcu`] rely on this assumption to safely use the
+/// raw pointers.
+///
+/// [`Rcu`]: super::Rcu
+pub unsafe trait OwnerPtr: 'static {
     /// The target type that this pointer refers to.
     // TODO: allow ?Sized
     type Target;
@@ -18,76 +27,68 @@ pub trait OwnerPtr: 'static {
 
     /// Converts to a raw pointer.
     ///
-    /// If `Self` owns the object that it refers to (e.g., `Box<_>`), then
+    /// Each call to `into_raw` must be paired with a call to `from_raw`
-    /// each call to `into_raw` must be paired with a call to `from_raw`
     /// in order to avoid memory leakage.
-    fn into_raw(self) -> *const Self::Target;
+    ///
+    /// The resulting raw pointer must be valid to be immutably accessed
+    /// or borrowed until `from_raw` is called.
+    fn into_raw(self) -> NonNull<Self::Target>;
 
     /// Converts back from a raw pointer.
     ///
     /// # Safety
     ///
-    /// The raw pointer must have been previously returned by a call to `into_raw`.
+    /// 1. The raw pointer must have been previously returned by a call to
-    unsafe fn from_raw(ptr: *const Self::Target) -> Self;
+    ///    `into_raw`.
+    /// 2. The raw pointer must not be used after calling `from_raw`.
+    ///
+    /// Note that the second point is a hard requirement: Even if the
+    /// resulting value has not (yet) been dropped, the pointer cannot be
+    /// used because it may break Rust aliasing rules (e.g., `Box<T>`
+    /// requires the pointer to be unique and thus _never_ aliased).
+    unsafe fn from_raw(ptr: NonNull<Self::Target>) -> Self;
 }
 
-impl<T: 'static> OwnerPtr for Box<T> {
+unsafe impl<T: 'static> OwnerPtr for Box<T> {
     type Target = T;
 
     fn new(value: Self::Target) -> Self {
         Box::new(value)
     }
 
-    fn into_raw(self) -> *const Self::Target {
+    fn into_raw(self) -> NonNull<Self::Target> {
-        Box::into_raw(self) as *const _
+        let ptr = Box::into_raw(self);
+
+        // SAFETY: The pointer representing a `Box` can never be NULL.
+        unsafe { NonNull::new_unchecked(ptr) }
     }
 
-    unsafe fn from_raw(ptr: *const Self::Target) -> Self {
+    unsafe fn from_raw(ptr: NonNull<Self::Target>) -> Self {
-        Box::from_raw(ptr as *mut _)
+        let ptr = ptr.as_ptr();
+
+        // SAFETY: The safety is upheld by the caller.
+        unsafe { Box::from_raw(ptr) }
     }
 }
 
-impl<T: 'static> OwnerPtr for Arc<T> {
+unsafe impl<T: 'static> OwnerPtr for Arc<T> {
     type Target = T;
 
     fn new(value: Self::Target) -> Self {
         Arc::new(value)
     }
 
-    fn into_raw(self) -> *const Self::Target {
+    fn into_raw(self) -> NonNull<Self::Target> {
-        Arc::into_raw(self)
+        let ptr = Arc::into_raw(self).cast_mut();
+
+        // SAFETY: The pointer representing an `Arc` can never be NULL.
+        unsafe { NonNull::new_unchecked(ptr) }
     }
 
-    unsafe fn from_raw(ptr: *const Self::Target) -> Self {
+    unsafe fn from_raw(ptr: NonNull<Self::Target>) -> Self {
-        Arc::from_raw(ptr)
+        let ptr = ptr.as_ptr().cast_const();
-    }
+
-}
+        // SAFETY: The safety is upheld by the caller.
-
+        unsafe { Arc::from_raw(ptr) }
-impl<P> OwnerPtr for Option<P>
-where
-    P: OwnerPtr,
-    // We cannot support fat pointers, e.g., when `Target: dyn Trait`.
-    // This is because Rust does not allow fat null pointers. Yet,
-    // we need the null pointer to represent `None`.
-    // See https://github.com/rust-lang/rust/issues/66316.
-    <P as OwnerPtr>::Target: Sized,
-{
-    type Target = P::Target;
-
-    fn new(value: Self::Target) -> Self {
-        Some(P::new(value))
-    }
-
-    fn into_raw(self) -> *const Self::Target {
-        self.map(|p| <P as OwnerPtr>::into_raw(p))
-            .unwrap_or(core::ptr::null())
-    }
-
-    unsafe fn from_raw(ptr: *const Self::Target) -> Self {
-        if ptr.is_null() {
-            Some(<P as OwnerPtr>::from_raw(ptr))
-        } else {
-            None
-        }
     }
 }