Address potential unsoundness during destruction

Author: arnodb

compile-tests/tests/compile-fail/reborrow.rs

@@ -2,7 +2,7 @@ extern crate ref_mut_stack;
 
 use ref_mut_stack::{ParkableRefMut, RefMutStack};
 
-fn main() -> () {
+fn main() {
     let mut root = ();
 
     struct Type<'a>(ParkableRefMut<'a, (), Self>);

justfile

@@ -21,8 +21,8 @@ test *args:
 compile-test *args:
     cargo test --all-features {{args}}
 
-miri:
-    cargo miri test --all-features
+miri *args:
+    cargo miri test --all-features {{args}}
 
 check_all:
     just stable

src/lib.rs

@@ -5,15 +5,15 @@ use std::{
 
 pub struct RefMutStack<'a, R, T> {
     root_ref: NonNull<R>,
-    stack: Vec<(T, NonNull<R>)>,
+    stack: SafeDropVec<(T, NonNull<R>)>,
     _a: std::marker::PhantomData<&'a ()>,
 }
 
 impl<'a, R, T> RefMutStack<'a, R, T> {
     pub fn new(r: &'a mut R) -> Self {
         Self {
             root_ref: NonNull::from_mut(r),
-            stack: Vec::new(),
+            stack: Vec::new().into(),
             _a: Default::default(),
         }
     }
@@ -27,6 +27,46 @@ impl<'a, R, T> RefMutStack<'a, R, T> {
     }
 }
 
+/// This is a Vec which will drop its elements in reverse order on destruction.
+///
+/// It ensures soundness of error cases when the elements holding the mutable references really
+/// want to access them during their destruction. See soundness integration tests.
+///
+/// Note that is would be an anti-pattern to use the references in destructors, but this wrapper
+/// addresses incorrect implementations.
+///
+/// Also note that the borrow checker will not allow us to implement `Drop` on [`RefMutStack`]
+/// itself because of its self referencing nature. The borrow checker does not complain on the
+/// implementation but on the use site when it needs to drop the stack and realizes there are
+/// conflicting lifetime requirements.
+struct SafeDropVec<T>(Vec<T>);
+
+impl<T> From<Vec<T>> for SafeDropVec<T> {
+    fn from(value: Vec<T>) -> Self {
+        Self(value)
+    }
+}
+
+impl<T> Drop for SafeDropVec<T> {
+    fn drop(&mut self) {
+        // Drop elements in reverse order.
+        while self.0.pop().is_some() {}
+    }
+}
+
+impl<T> Deref for SafeDropVec<T> {
+    type Target = Vec<T>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> DerefMut for SafeDropVec<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
 pub struct ParkableRefMut<'a, R, T> {
     r: NonNull<R>,
     stack: NonNull<RefMutStack<'a, R, T>>,

tests/impl_drop.rs

@@ -0,0 +1,80 @@
+use ref_mut_stack::{ParkableRefMut, RefMutStack};
+
+// We want to provide a way to the user to use the references in destructors.
+//
+// But because of the existence of such a destructor, the reference needs to be detached from
+// `self` while unparking. This is why it is wrapped with an `Option`.
+
+struct Builder<'a>(Option<ParkableRefMut<'a, usize, Self>>, usize);
+
+impl<'a> Builder<'a> {
+    fn build(mut self) -> Option<Self> {
+        let r = self.0.take().unwrap();
+        r.unpark()
+    }
+}
+
+impl<'a> Drop for Builder<'a> {
+    fn drop(&mut self) {
+        if let Some(r) = self.0.as_mut() {
+            **r = self.1
+        };
+    }
+}
+
+#[test]
+pub fn test_sound_incomplete_unstacking() {
+    let mut root = 12;
+
+    {
+        let mut stack = RefMutStack::<usize, Builder>::new(&mut root);
+        let mut b1 = Builder(Some(stack.borrow_mut()), 100);
+        let mut b2 = Builder(Some(b1.0.as_mut().unwrap().parker().park(b1, |r| r)), 200);
+        let mut b3 = Builder(Some(b2.0.as_mut().unwrap().parker().park(b2, |r| r)), 300);
+        let b4 = Builder(Some(b3.0.as_mut().unwrap().parker().park(b3, |r| r)), 400);
+
+        let Some(_b3) = b4.build() else {
+            panic!("b4 build should return b3")
+        };
+        // `b3`, `b2` and `b1` are not built.
+        //
+        // The last 2 are still in the stack and need to be dropped in order for the process to be
+        // sound.
+        //
+        // Everything is dropped here.
+    }
+
+    // If not 100 that means the destructors are not called in the right order, which indicates
+    // unsound behaviour (detected by Miri).
+    assert_eq!(root, 100);
+}
+
+#[test]
+pub fn test_sound_unwinding() {
+    // Need a lock to cross the catch_unwind scope safely
+    let root = std::sync::Mutex::new(12);
+
+    std::panic::catch_unwind(|| {
+        let mut root = root.lock().unwrap();
+
+        let mut stack = RefMutStack::<usize, Builder>::new(&mut root);
+        let mut b1 = Builder(Some(stack.borrow_mut()), 100);
+        let mut b2 = Builder(Some(b1.0.as_mut().unwrap().parker().park(b1, |r| r)), 200);
+        let mut b3 = Builder(Some(b2.0.as_mut().unwrap().parker().park(b2, |r| r)), 300);
+        let b4 = Builder(Some(b3.0.as_mut().unwrap().parker().park(b3, |r| r)), 400);
+
+        // Drop root to make sure the lock is not poisoned
+        drop(b4);
+        drop(root);
+
+        // 3 builders are still in the stack and need to be dropped in order for the process to be
+        // sound.
+
+        panic!("intentional panic");
+    })
+    .unwrap_err();
+
+    // If not 100 that means the destructors are not called in the right order, which indicates
+    // unsound behaviour (detected by Miri).
+    assert_eq!(*root.lock().unwrap(), 100);
+}