diff --git a/Cargo.toml b/Cargo.toml index 202e737..4a7ae87 100644 --- a/Cargo.toml +++ b/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 "] 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 } diff --git a/src/cc.rs b/src/cc.rs index 4fa909c..291995a 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -77,6 +77,7 @@ impl Cc { 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 Cc { 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 Drop for Cc { // 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 Drop for Cc { "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 diff --git a/src/lib.rs b/src/lib.rs index a5947aa..62c5f61 100644 --- a/src/lib.rs +++ b/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); } }); diff --git a/src/tests/weak/mod.rs b/src/tests/weak/mod.rs index ef2013d..0f16e50 100644 --- a/src/tests/weak/mod.rs +++ b/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>>, + } + + 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(); +} diff --git a/src/weak/mod.rs b/src/weak/mod.rs index 361a1b1..5d39441 100644 --- a/src/weak/mod.rs +++ b/src/weak/mod.rs @@ -286,11 +286,13 @@ let cyclic = Cc::new_cyclic(|weak| { impl Drop for PanicGuard { 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); }); } diff --git a/src/weak/weak_counter_marker.rs b/src/weak/weak_counter_marker.rs index fa6bffa..53d2466 100644 --- a/src/weak/weak_counter_marker.rs +++ b/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 { diff --git a/tests/cc.rs b/tests/cc.rs index a5c139c..62cb90d 100644 --- a/tests/cc.rs +++ b/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};