Merge pull request #33 from frengor/dev

* Update to 0.6.1
* Fix use-after-free related to weak pointers

Author: frengor

Cargo.toml

@@ -14,7 +14,7 @@ edition.workspace = true
 members = ["derive"]
 
 [workspace.package]
-version = "0.6.0" # Also update in [dependencies.rust-cc-derive.version]
+version = "0.6.1" # Also update in [dependencies.rust-cc-derive.version]
 authors = ["fren_gor <goro@frengor.com>"]
 repository = "https://github.com/frengor/rust-cc"
 categories = ["memory-management", "no-std"]
@@ -49,7 +49,7 @@ std = ["slotmap?/std", "thiserror/std"]
 pedantic-debug-assertions = []
 
 [dependencies]
-rust-cc-derive = { path = "./derive", version = "=0.6.0", optional = true }
+rust-cc-derive = { path = "./derive", version = "=0.6.1", optional = true }
 slotmap = {  version = "1.0", optional = true }
 thiserror = { version = "1.0", package = "thiserror-core", default-features = false }
 

src/cc.rs

@@ -77,6 +77,7 @@ impl<T: Trace> Cc<T> {
         let cc = ManuallyDrop::new(self); // Never drop the Cc
 
         if cc.strong_count() != 1 {
+            // cc is not unique
             // No need to access the state here
             return Err(ManuallyDrop::into_inner(cc));
         }
@@ -94,14 +95,15 @@ impl<T: Trace> Cc<T> {
 
             remove_from_list(cc.inner.cast());
 
-            // Disable upgrading weak ptrs
+            // SAFETY: cc is unique
+            let t = unsafe { ptr::read(cc.inner().get_elem()) };
+            let layout = cc.inner().layout();
+
+            // Disable upgrading weak ptrs (after having read the layout)
             #[cfg(feature = "weak-ptrs")]
             cc.inner().drop_metadata();
-            // There's no reason here to call CounterMarker::set_dropped, since the pointed value will not be dropped
+            // There's no reason here to call CounterMarker::set_dropped, since the pointed value will not be dropped and the allocation will be freed
 
-            // SAFETY: cc is unique and no weak pointer can be upgraded
-            let t = unsafe { ptr::read(cc.inner().get_elem()) };
-            let layout = cc.inner().layout();
             // SAFETY: cc is unique, is not inside any list and no weak pointer can be upgraded
             unsafe {
                 cc_dealloc(cc.inner, layout, state);
@@ -304,8 +306,6 @@ impl<T: ?Sized + Trace> Drop for Cc<T> {
                     // Set the object as dropped before dropping and deallocating it
                     // This feature is used only in weak pointers, so do this only if they're enabled
                     self.counter_marker().set_dropped(true);
-
-                    self.inner().drop_metadata();
                 }
 
                 // SAFETY: we're the only one to have a pointer to this allocation
@@ -318,6 +318,10 @@ impl<T: ?Sized + Trace> Drop for Cc<T> {
                         "Trying to deallocate a CcBox with a reference counter > 0"
                     );
 
+                    // Free metadata (the layout has already been read). Necessary only with weak pointers enabled
+                    #[cfg(feature = "weak-ptrs")]
+                    self.inner().drop_metadata();
+
                     cc_dealloc(self.inner, layout, state);
                 }
                 // _dropping_guard is dropped here, resetting state.dropping

src/lib.rs

@@ -137,7 +137,7 @@ cleanable.clean();
 #![cfg_attr(not(feature = "std"), no_std)]
 
 #![deny(rustdoc::broken_intra_doc_links)]
-#![allow(clippy::thread_local_initializer_can_be_made_const)]
+#![allow(clippy::missing_const_for_thread_local)]
 
 #[cfg(all(not(feature = "std"), not(feature = "nightly")))]
 compile_error!("Feature \"std\" cannot be disabled without enabling feature \"nightly\" (due to #[thread_local] not being stable).");
@@ -403,9 +403,6 @@ fn deallocate_list(to_deallocate_list: LinkedList, state: &State) {
         unsafe {
             debug_assert!(ptr.as_ref().counter_marker().is_in_list());
 
-            #[cfg(feature = "weak-ptrs")]
-            ptr.as_ref().drop_metadata();
-
             CcBox::drop_inner(ptr.cast());
         };
 
@@ -428,6 +425,12 @@ fn deallocate_list(to_deallocate_list: LinkedList, state: &State) {
         //         and then the allocation gets deallocated immediately after.
         unsafe {
             let layout = ptr.as_ref().layout();
+
+            // Free metadata only after having read the layout from the vtable
+            // Necessary only with weak pointers enabled
+            #[cfg(feature = "weak-ptrs")]
+            ptr.as_ref().drop_metadata();
+
             cc_dealloc(ptr, layout, state);
         }
     });

src/tests/weak/mod.rs

@@ -411,3 +411,38 @@ fn weak_new_ptr_eq() {
     assert!(Weak::ptr_eq(&cc.downgrade(), &cc.downgrade()));
     assert!(!Weak::ptr_eq(&cc.downgrade(), &other_cc.downgrade()));
 }
+
+#[test]
+fn drop_zero_weak_counter() {
+    reset_state();
+
+    let cc = Cc::new(5u32);
+    let _ = cc.downgrade();
+    drop(cc);
+}
+
+#[test]
+fn cyclic_drop_zero_weak_counter() {
+    reset_state();
+
+    struct Cyclic {
+        cyclic: RefCell<Option<Cc<Cyclic>>>,
+    }
+
+    unsafe impl Trace for Cyclic {
+        fn trace(&self, ctx: &mut Context<'_>) {
+            self.cyclic.trace(ctx);
+        }
+    }
+
+    impl Finalize for Cyclic {}
+
+    let cc = Cc::new(Cyclic {
+        cyclic: RefCell::new(None),
+    });
+    *cc.cyclic.borrow_mut() = Some(cc.clone());
+
+    let _ = cc.downgrade();
+    drop(cc);
+    collect_cycles();
+}

src/weak/mod.rs

@@ -286,11 +286,13 @@ let cyclic = Cc::new_cyclic(|weak| {
         impl<T: Trace> Drop for PanicGuard<T> {
             fn drop(&mut self) {
                 unsafe {
-                    // Deallocate only the metadata allocation
+                    let layout = self.invalid_cc.as_ref().layout();
+
+                    // Deallocate only the metadata allocation (the layout has already been read)
                     self.invalid_cc.as_ref().drop_metadata();
+
                     // Deallocate the CcBox. Use try_state to avoid panicking inside a Drop
                     let _ = try_state(|state| {
-                        let layout = self.invalid_cc.as_ref().layout();
                         cc_dealloc(self.invalid_cc, layout, state);
                     });
                 }

src/weak/weak_counter_marker.rs

@@ -16,7 +16,7 @@ pub(crate) const MAX: u16 = !ACCESSIBLE_MASK; // First 15 bits to 1
 /// +-----------+-----------+
 /// ```
 ///
-/// * `A` is `1` when the `Cc` is accessible, `0` otherwise
+/// * `A` is `1` when the `CcBox` is accessible (i.e., not deallocated), `0` otherwise
 /// * `B` is the weak counter
 #[derive(Clone, Debug)]
 pub(crate) struct WeakCounterMarker {

tests/cc.rs

@@ -1,4 +1,4 @@
-#![allow(clippy::thread_local_initializer_can_be_made_const)]
+#![allow(clippy::missing_const_for_thread_local)]
 
 use std::cell::{Cell, RefCell};
 use std::rc::{Rc, Weak};