From 0db3b7efbc86dc02144f63d86450c97a494313ad Mon Sep 17 00:00:00 2001 From: VladimirK <47093374+VladimirKras@users.noreply.github.com> Date: Sun, 4 Oct 2026 21:20:57 +0300 Subject: [PATCH] Fix GC ownership and release freelist payloads before caching Traverse function builtins, member descriptor declaring types, and weakref callbacks so unreachable cycles can be collected. Keep referents weak and read callbacks under their existing stripe lock. Destroy thread-local freelist payloads before caching, while TLS is still available. Reinitialize the payload on reuse and release cached headers with the complete allocation layout on thread exit. Preserve initialized payloads for custom freelists such as the MemoryError pool. Add regressions for live references, cycle collection, weakref callbacks, and releasing a deleted slice's stop argument. Assisted-by: Codex:gpt-6 --- crates/vm/src/builtins/complex.rs | 3 +- crates/vm/src/builtins/descriptor.rs | 9 ++- crates/vm/src/builtins/dict.rs | 3 +- crates/vm/src/builtins/float.rs | 3 +- crates/vm/src/builtins/function.rs | 2 + crates/vm/src/builtins/int.rs | 3 +- crates/vm/src/builtins/list.rs | 3 +- crates/vm/src/builtins/range.rs | 3 +- crates/vm/src/builtins/slice.rs | 5 +- crates/vm/src/builtins/tuple.rs | 3 +- crates/vm/src/frame.rs | 3 +- crates/vm/src/object/core.rs | 75 +++++++++++++------ crates/vm/src/object/payload.rs | 13 +++- .../snippets/stdlib_gc_function_builtins.py | 28 +++++++ .../snippets/stdlib_gc_member_descriptor.py | 24 ++++++ .../snippets/stdlib_gc_slice_payload.py | 21 ++++++ .../snippets/stdlib_gc_weakref_callback.py | 44 +++++++++++ 17 files changed, 209 insertions(+), 36 deletions(-) create mode 100644 extra_tests/snippets/stdlib_gc_function_builtins.py create mode 100644 extra_tests/snippets/stdlib_gc_member_descriptor.py create mode 100644 extra_tests/snippets/stdlib_gc_slice_payload.py create mode 100644 extra_tests/snippets/stdlib_gc_weakref_callback.py diff --git a/crates/vm/src/builtins/complex.rs b/crates/vm/src/builtins/complex.rs index f471c43c580..079b98046bc 100644 --- a/crates/vm/src/builtins/complex.rs +++ b/crates/vm/src/builtins/complex.rs @@ -41,6 +41,7 @@ thread_local! { impl PyPayload for PyComplex { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -53,7 +54,7 @@ impl PyPayload for PyComplex { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/descriptor.rs b/crates/vm/src/builtins/descriptor.rs index b3189e592a8..f4bea6b351c 100644 --- a/crates/vm/src/builtins/descriptor.rs +++ b/crates/vm/src/builtins/descriptor.rs @@ -736,7 +736,7 @@ impl PyMemberSpec { } // = PyMemberDescrObject -#[pyclass(name = "member_descriptor", module = false)] +#[pyclass(name = "member_descriptor", module = false, traverse = "manual")] #[derive(Debug)] pub struct PyMemberDescriptor { #[pymember(name = "__objclass__", path = "typ")] @@ -746,6 +746,13 @@ pub struct PyMemberDescriptor { pub access: MemberAccess, } +// SAFETY: common.typ is owned; the other fields contain no owned Python objects. +unsafe impl crate::object::Traverse for PyMemberDescriptor { + fn traverse(&self, tracer_fn: &mut crate::object::TraverseFn<'_>) { + tracer_fn(self.common.typ.as_object()); + } +} + impl PyMemberDescriptor { /// Byte offset of an object-pointer member. `None` for bool, float, and /// struct-sequence indexes, which slot specialization must not treat as cells. diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d709521ea6d..ae11932d7e7 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -69,6 +69,7 @@ thread_local! { impl PyPayload for PyDict { const MAX_FREELIST: usize = 80; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -81,7 +82,7 @@ impl PyPayload for PyDict { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/float.rs b/crates/vm/src/builtins/float.rs index 78d3bef7794..03b6fc92337 100644 --- a/crates/vm/src/builtins/float.rs +++ b/crates/vm/src/builtins/float.rs @@ -48,6 +48,7 @@ thread_local! { impl PyPayload for PyFloat { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -60,7 +61,7 @@ impl PyPayload for PyFloat { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index c6c4aaf2b79..fde1f6ef346 100644 --- a/crates/vm/src/builtins/function.rs +++ b/crates/vm/src/builtins/function.rs @@ -99,6 +99,8 @@ fn next_func_version() -> u32 { unsafe impl Traverse for PyFunction { fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { self.globals.traverse(tracer_fn); + // Both fields own a reference, even when builtins aliases globals. + self.builtins.traverse(tracer_fn); if let Some(closure) = self.closure.as_ref() { // Visit the closure tuple itself as an edge, not its cells: the // tuple is a tracked object that can join a reference cycle, and diff --git a/crates/vm/src/builtins/int.rs b/crates/vm/src/builtins/int.rs index d6799474582..d35196e0cea 100644 --- a/crates/vm/src/builtins/int.rs +++ b/crates/vm/src/builtins/int.rs @@ -58,6 +58,7 @@ thread_local! { impl PyPayload for PyInt { const MAX_FREELIST: usize = 100; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -74,7 +75,7 @@ impl PyPayload for PyInt { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index d6a2c1669a7..8dec0d5d6a1 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -90,6 +90,7 @@ thread_local! { impl PyPayload for PyList { const MAX_FREELIST: usize = 80; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -102,7 +103,7 @@ impl PyPayload for PyList { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/range.rs b/crates/vm/src/builtins/range.rs index e8d2c30dbce..a8db4fab6ef 100644 --- a/crates/vm/src/builtins/range.rs +++ b/crates/vm/src/builtins/range.rs @@ -80,6 +80,7 @@ thread_local! { impl PyPayload for PyRange { const MAX_FREELIST: usize = 6; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -92,7 +93,7 @@ impl PyPayload for PyRange { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/slice.rs b/crates/vm/src/builtins/slice.rs index 8cc53fd16c1..3728d232869 100644 --- a/crates/vm/src/builtins/slice.rs +++ b/crates/vm/src/builtins/slice.rs @@ -40,7 +40,7 @@ unsafe impl crate::object::Traverse for PySlice { out.push(start); } // stop is not Option, so it will be freed when payload is dropped - // (via drop_in_place on freelist pop, or Box::from_raw on dealloc) + // (before freelist caching, or during deallocation) if let Some(step) = self.step.take() { out.push(step); } @@ -54,6 +54,7 @@ thread_local! { impl PyPayload for PySlice { const MAX_FREELIST: usize = 1; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -66,7 +67,7 @@ impl PyPayload for PySlice { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/builtins/tuple.rs b/crates/vm/src/builtins/tuple.rs index a1c06e49106..4168b6564d4 100644 --- a/crates/vm/src/builtins/tuple.rs +++ b/crates/vm/src/builtins/tuple.rs @@ -118,6 +118,7 @@ thread_local! { impl PyPayload for PyTuple { const MAX_FREELIST: usize = 2000; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; #[inline] fn class(ctx: &Context) -> &'static Py { @@ -130,7 +131,7 @@ impl PyPayload for PyTuple { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index dadc0476be8..fa71f80e4b5 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -1615,6 +1615,7 @@ thread_local! { impl PyPayload for FrameObject { const MAX_FREELIST: usize = 200; const HAS_FREELIST: bool = true; + const FREELIST_HAS_PAYLOAD: bool = false; // Ordinary call frames are created untracked and only enter the GC when // they escape (see `release_datastack_frame`); generator/coroutine frames // are tracked explicitly at creation in `invoke_with_locals`. @@ -1631,7 +1632,7 @@ impl PyPayload for FrameObject { .try_with(|fl| { let mut list = fl.take(); let stored = if list.len() < Self::MAX_FREELIST { - list.push(obj); + unsafe { list.push(obj) }; true } else { false diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index eb7edc93cca..a59857d66d4 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -1015,7 +1015,7 @@ unsafe impl Link for WeakLink { } // PyWeakReference: each weakref holds a direct pointer to its referent. -#[pyclass(name = "ReferenceType", module = "weakref")] +#[pyclass(name = "ReferenceType", module = "weakref", traverse = "manual")] #[derive(Debug)] pub struct PyWeak { pointers: Pointers>, @@ -1035,6 +1035,21 @@ cfg_select! { _ => {} } +// SAFETY: the stripe lock protects the owned callback; wr_object is a weak pointer. +unsafe impl Traverse for PyWeak { + fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { + let obj_ptr = self.wr_object.load(Ordering::Acquire); + if obj_ptr.is_null() { + return; + } + let _lock = weakref_lock::lock(obj_ptr as usize); + if !self.wr_object.load(Ordering::Relaxed).is_null() { + // SAFETY: the callback is read under the same lock used to consume it. + unsafe { (&*self.callback.get()).traverse(tracer_fn) }; + } + } +} + impl PyWeak { /// _PyWeakref_GET_REF: attempt to upgrade the weakref to a strong reference. pub(crate) fn upgrade(&self) -> Option { @@ -1247,9 +1262,11 @@ impl Py { /// object it points at until its own `Drop` unlinks it, and a thread /// walking that list reads the class off every node it passes, so the /// class has to outlive the payload. - unsafe fn drop_fields(ptr: *mut Self) { + unsafe fn drop_fields(ptr: *mut Self) { unsafe { - core::ptr::drop_in_place(&raw mut (*ptr).payload); + if DROP_PAYLOAD { + core::ptr::drop_in_place(&raw mut (*ptr).payload); + } core::ptr::drop_in_place(&raw mut (*ptr).typ); } } @@ -1260,8 +1277,15 @@ impl Py { /// # Safety /// `ptr` must be a valid pointer from `Py::new` and must not be used after this call. unsafe fn dealloc(ptr: *mut Self) { + unsafe { Self::dealloc_inner::(ptr) }; + } + + // Freelist entries have already had their payload destroyed. + unsafe fn dealloc_inner(ptr: *mut Self) { unsafe { - let (flags, member_count) = (*ptr).read_type_flags(); + // Read the initialized header, including for cached allocations + // whose payload has already been destroyed. + let (flags, member_count) = (*(ptr as *const PyObject)).0.read_type_flags(); let has_ext = flags.contains(&crate::types::PyTypeFlags::HAS_DICT) || member_count > 0; let has_weakref = flags.contains(&crate::types::PyTypeFlags::HAS_WEAKREF); // Objects published to lock-free caches keep their memory mapped @@ -1293,7 +1317,7 @@ impl Py { let alloc_ptr = (ptr as *mut u8).sub(inner_offset); - Self::drop_fields(ptr); + Self::drop_fields::(ptr); // Drop member cells, then ObjExt. WeakRefList is in front of the cells. let mut cursor = alloc_ptr; @@ -1321,10 +1345,10 @@ impl Py { } } else if published { let layout = core::alloc::Layout::new::(); - Self::drop_fields(ptr); + Self::drop_fields::(ptr); crate::object::qsbr::free_delayed(ptr as *mut u8, layout); } else { - Self::drop_fields(ptr); + Self::drop_fields::(ptr); // The fields are gone; the box is only here to free the memory // the matching `Box::new` in `new` allocated. drop(Box::from_raw(ptr.cast::>())); @@ -1459,15 +1483,28 @@ impl Py { /// Thread-local freelist storage for reusing object allocations. /// -/// Wraps a `Vec<*mut PyObject>`. On thread teardown, `Drop` frees raw -/// `Py` allocations without running payload destructors to avoid -/// accessing already-destroyed thread-local storage (GC state, other freelists). +/// Cached allocations retain their header, but not their payload. Payloads +/// are destroyed before caching, while thread-local deallocation state is +/// available. Thread teardown therefore only releases the allocation and its +/// reference to the static base class. pub(crate) struct FreeList { items: Vec<*mut PyObject>, _marker: core::marker::PhantomData, } impl FreeList { + /// Cache a dead allocation after destroying its payload. + /// + /// # Safety + /// `ptr` must point to an untracked, unpublished, exact base-type `Py` + /// with no outstanding payload borrows. Its payload must be initialized. + pub(crate) unsafe fn push(&mut self, ptr: *mut PyObject) { + debug_assert!(!T::FREELIST_HAS_PAYLOAD); + // Destruction can reenter allocation; publish the husk only afterwards. + unsafe { core::ptr::drop_in_place(&raw mut (*(ptr as *mut Py)).payload) }; + self.items.push(ptr); + } + pub(crate) const fn new() -> Self { Self { items: Vec::new(), @@ -1484,16 +1521,10 @@ impl Default for FreeList { impl Drop for FreeList { fn drop(&mut self) { - // During thread teardown, we cannot safely run destructors on cached - // objects because their Drop impls may access thread-local storage - // (GC state, other freelists) that is already destroyed. - // Instead, free just the raw allocation. The payload's heap fields - // (BigInt, PyObjectRef, etc.) are leaked, but this is bounded by - // MAX_FREELIST per type per thread. for ptr in self.items.drain(..) { - unsafe { - alloc::alloc::dealloc(ptr as *mut u8, core::alloc::Layout::new::>()); - } + // The payload is gone and the remaining class reference points to + // a static base type, so no Python destruction can run here. + unsafe { Py::::dealloc_inner::(ptr as *mut Py) }; } } } @@ -2827,8 +2858,10 @@ impl PyRef { unsafe { core::ptr::write(&mut (*inner).ref_count, RefCount::new()); (*inner).gc_bits.store(0, Ordering::Relaxed); - core::ptr::drop_in_place(&mut (*inner).payload); - core::ptr::write(&mut (*inner).payload, payload); + if T::FREELIST_HAS_PAYLOAD { + core::ptr::drop_in_place(&raw mut (*inner).payload); + } + core::ptr::write(&raw mut (*inner).payload, payload); // Freelist only stores exact base types (push-side filter), // but subtypes sharing the same Rust payload (e.g. structseq) // may pop entries. Update typ if it differs. diff --git a/crates/vm/src/object/payload.rs b/crates/vm/src/object/payload.rs index a5a4bf35b7f..09b6c86eb85 100644 --- a/crates/vm/src/object/payload.rs +++ b/crates/vm/src/object/payload.rs @@ -96,13 +96,18 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static { /// race conditions when the object is reused. const HAS_FREELIST: bool = false; + /// Whether cached allocations retain an initialized payload. + /// `FreeList` destroys payloads before caching and requires false. + const FREELIST_HAS_PAYLOAD: bool = true; + /// Maximum number of objects to keep in the freelist. const MAX_FREELIST: usize = 0; /// Try to push a dead object onto this type's freelist for reuse. /// Returns true if the object was stored (caller must NOT free the memory). /// Called after tp_clear, so the payload is a cleared husk; implementations - /// must not rely on its pre-clear contents. + /// must not rely on its pre-clear contents. If `FREELIST_HAS_PAYLOAD` is + /// false, destroy the payload before publishing the allocation for reuse. /// /// # Safety /// `obj` must be a valid pointer to a `Py` with refcount 0 @@ -114,13 +119,13 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static { } /// Try to pop a pre-allocated object from this type's freelist. - /// The returned pointer still has the old payload; the caller must + /// The caller must /// reinitialize `ref_count`, `gc_bits`, and `payload`. /// /// # Safety /// The returned pointer (if Some) must point to a valid `Py` - /// whose payload is still initialized from a previous allocation. The caller - /// will drop and overwrite `payload` before reuse. + /// whose payload is initialized if and only if `FREELIST_HAS_PAYLOAD` is + /// true. The caller will destroy it if initialized, then write a new payload. #[inline] unsafe fn freelist_pop(_payload: &Self) -> Option> { None diff --git a/extra_tests/snippets/stdlib_gc_function_builtins.py b/extra_tests/snippets/stdlib_gc_function_builtins.py new file mode 100644 index 00000000000..8f6b429142d --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_function_builtins.py @@ -0,0 +1,28 @@ +"""Functions must report every owned builtins reference to the cycle collector.""" + +import gc +import weakref + + +def check_function_cycle(shared_namespace): + namespace = {} + builtins = namespace if shared_namespace else {} + namespace["__builtins__"] = builtins + exec("def temporary(): return 42", namespace) + keeper = namespace["temporary"] + builtins["temporary"] = keeper + reference = weakref.ref(keeper) + del namespace, builtins + + gc.collect() + assert reference() is keeper + assert keeper() == 42 + + del keeper + gc.collect() + assert reference() is None + + +check_function_cycle(shared_namespace=False) +check_function_cycle(shared_namespace=True) +print("ok") diff --git a/extra_tests/snippets/stdlib_gc_member_descriptor.py b/extra_tests/snippets/stdlib_gc_member_descriptor.py new file mode 100644 index 00000000000..0c1862e847e --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_member_descriptor.py @@ -0,0 +1,24 @@ +"""A slot descriptor owns its class, including when the class becomes garbage.""" + +import gc +import weakref + + +def create(): + class Temporary: + __slots__ = ("value",) + + return weakref.ref(Temporary), Temporary.value + + +reference, descriptor = create() +gc.collect() +assert reference() is descriptor.__objclass__ +instance = descriptor.__objclass__() +descriptor.__set__(instance, 42) +assert descriptor.__get__(instance) == 42 + +del instance, descriptor +gc.collect() +assert reference() is None +print("ok") diff --git a/extra_tests/snippets/stdlib_gc_slice_payload.py b/extra_tests/snippets/stdlib_gc_slice_payload.py new file mode 100644 index 00000000000..735758f6f9e --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_slice_payload.py @@ -0,0 +1,21 @@ +import gc +import weakref + + +class Target: + pass + + +target = Target() +reference = weakref.ref(target) +value = slice(None, target, None) +del target +gc.collect() +assert reference() is value.stop + +del value +gc.collect() +assert reference() is None, "a cached slice retained its stop argument" + +# Reusing cached storage must initialize a fresh payload. +assert slice(1, 7, 2).indices(10) == (1, 7, 2) diff --git a/extra_tests/snippets/stdlib_gc_weakref_callback.py b/extra_tests/snippets/stdlib_gc_weakref_callback.py new file mode 100644 index 00000000000..0909403f38b --- /dev/null +++ b/extra_tests/snippets/stdlib_gc_weakref_callback.py @@ -0,0 +1,44 @@ +"""Weak references own their callbacks, but never own their referents.""" + +import gc +import weakref + + +class Target: + pass + + +class CallableTarget: + def __call__(self): + return 42 + + +def check_callback_cycle(target, make_reference): + def create(): + holder = [] + + def callback(reference): + holder.append(reference) + + reference = make_reference(target, callback) + holder.append(reference) + return weakref.ref(callback) + + reference = create() + gc.collect() + assert reference() is None + + +check_callback_cycle(Target(), weakref.ref) +check_callback_cycle(Target(), weakref.proxy) +check_callback_cycle(CallableTarget(), weakref.proxy) + +called = [] +target = Target() +target.cycle = target +reference = weakref.ref(target, lambda ref: called.append(ref() is None)) +del target +gc.collect() +assert reference() is None +assert called == [True] +print("ok")