fix: garbage collection issues for #[pyclass(dict)]

newsfragments/6206.fixed.1.md

@@ -0,0 +1 @@
+Fixed a crash (process abort) inside a `#[pyclass]`'s GC traversal when the traversal is stopped early, for example when `gc.get_referrers` finds data held by a `#[pyclass]` with a `#[pyclass]` base class.

newsfragments/6206.fixed.md

@@ -0,0 +1 @@
+Fix reference cycles through the `__dict__` of a `#[pyclass(dict)]` never being collected, leaking the instance. Such classes are now GC types whose `tp_traverse` / `tp_clear` visit and clear the instance `__dict__`.

pyo3-macros-backend/src/pymethod.rs

@@ -515,7 +515,7 @@ fn impl_clear_slot(cls: &syn::Type, spec: &FnSpec<'_>, ctx: &Ctx) -> syn::Result
         pub unsafe extern "C" fn __pymethod___clear____(
             _slf: *mut #pyo3_path::ffi::PyObject,
         ) -> ::std::ffi::c_int {
-            #pyo3_path::impl_::pymethods::_call_clear(_slf, |py, _slf| {
+            #pyo3_path::impl_::pymethods::_call_clear::<#cls>(_slf, |py, _slf| {
                 #holders
                 let result = #fncall;
                 let result = #pyo3_path::impl_::wrap::converter(&result).wrap(result)?;

src/impl_/pyclass.rs

@@ -64,7 +64,16 @@ pub trait PyClassDict: sealed::Sealed {
     const INIT: Self;
     /// Empties the dictionary of its key-value pairs.
     #[inline]
-    fn clear_dict(&mut self, _py: Python<'_>) {}
+    fn clear_dict(&self, _py: Python<'_>) {}
+    /// Visits the `__dict__`, if any, on behalf of `tp_traverse`.
+    ///
+    /// # Safety
+    /// - Must only be called from a `tp_traverse` implementation, passing that
+    ///   implementation's `visit` and `arg` unchanged.
+    #[inline]
+    unsafe fn traverse_dict(&self, _visit: ffi::visitproc, _arg: *mut c_void) -> c_int {
+        0
+    }
 }
 
 /// Represents the `__weakref__` field for `#[pyclass]`.
@@ -100,11 +109,19 @@ pub struct PyClassDictSlot(*mut ffi::PyObject);
 impl PyClassDict for PyClassDictSlot {
     const INIT: Self = Self(core::ptr::null_mut());
     #[inline]
-    fn clear_dict(&mut self, _py: Python<'_>) {
+    fn clear_dict(&self, _py: Python<'_>) {
         if !self.0.is_null() {
             unsafe { ffi::PyDict_Clear(self.0) }
         }
     }
+    #[inline]
+    unsafe fn traverse_dict(&self, visit: ffi::visitproc, arg: *mut c_void) -> c_int {
+        if self.0.is_null() {
+            0
+        } else {
+            unsafe { visit(self.0, arg) }
+        }
+    }
 }
 
 /// Actual weakref field, which holds the pointer to `__weakref__`.

src/impl_/pymethods.rs

@@ -5,6 +5,7 @@ use crate::exceptions::PyStopAsyncIteration;
 use crate::impl_::callback::IntoPyCallbackOutput;
 use crate::impl_::panic::PanicTrap;
 use crate::impl_::pycell::PyClassObjectBaseLayout;
+use crate::impl_::pyclass::PyClassDict as _;
 use crate::internal::get_slot::{get_slot, TP_BASE, TP_CLEAR, TP_TRAVERSE};
 use crate::internal::pyclass_init::PyClassInit;
 use crate::internal::state::ForbidAttaching;
@@ -362,9 +363,38 @@ where
     // token to the user code and forbid safe methods for attaching.
     // (This includes enforcing the `&self` method receiver as e.g. `PyRef<Self>` could
     // reconstruct a Python token via `PyRef::py`.)
+    //
+    // The traversal lives in `traverse_impl` so that it can return early: `trap` is armed until
+    // `disarm` below, and dropping it armed panics, which aborts out of `tp_traverse`.
     let trap = PanicTrap::new("uncaught panic inside __traverse__ handler");
     let lock = ForbidAttaching::during_traverse();
 
+    let retval = unsafe { traverse_impl(slf, impl_, visit, arg, current_traverse) };
+
+    // Drop lock before trap just in case dropping lock panics
+    drop(lock);
+    trap.disarm();
+    retval
+}
+
+/// Visits the base type, the instance `__dict__` and the pyclass's own data, stopping as soon as
+/// one of them returns non-zero.
+///
+/// # Safety
+/// - `slf` must be a valid pointer to an instance of `T`.
+/// - Must only be called from `_call_traverse`, which holds the `PanicTrap` and `ForbidAttaching`
+///   lock this relies on.
+unsafe fn traverse_impl<T>(
+    slf: *mut ffi::PyObject,
+    impl_: fn(&T, PyVisit<'_>) -> Result<(), PyTraverseError>,
+    visit: ffi::visitproc,
+    arg: *mut c_void,
+    current_traverse: ffi::traverseproc,
+) -> c_int
+where
+    T: PyClass,
+{
+    // A non-zero return means a `visitproc` has asked us to stop the traversal.
     let super_retval = unsafe { call_super_traverse(slf, visit, arg, current_traverse) };
     if super_retval != 0 {
         return super_retval;
@@ -374,39 +404,45 @@ where
     // traversal is running so no mutations can occur.
     let class_object: &<T as PyClassImpl>::Layout = unsafe { &*slf.cast() };
 
-    let retval =
-    // `#[pyclass(unsendable)]` types can only be deallocated by their own thread, so
-    // do not traverse them if not on their owning thread :(
-    if class_object.check_threadsafe().is_ok()
-    // ... and we cannot traverse a type which might be being mutated by a Rust thread
-    && class_object.borrow_checker().try_borrow().is_ok() {
-        struct TraverseGuard<'a, T: PyClassImpl>(&'a T::Layout);
-        impl<T: PyClassImpl> Drop for TraverseGuard<'_, T> {
-            fn drop(&mut self) {
-                self.0.borrow_checker().release_borrow()
-            }
-        }
-
-        // `.try_borrow()` above created a borrow, we need to release it when we're done
-        // traversing the object. This allows us to read `instance` safely.
-        let _guard = TraverseGuard::<T>(class_object);
-        let instance = unsafe {&*class_object.contents().value.get()};
+    // The `__dict__` is not Rust data, so it is visited without the thread and borrow checks
+    // below: it must stay reachable to the GC even when the pyclass data cannot be traversed.
+    let dict_retval = unsafe { class_object.contents().dict.traverse_dict(visit, arg) };
+    if dict_retval != 0 {
+        return dict_retval;
+    }
 
-        let visit = PyVisit { visit, arg, _guard: PhantomData };
+    // `#[pyclass(unsendable)]` types can only be deallocated by their own thread, so do not
+    // traverse them if not on their owning thread :(
+    // ... and we cannot traverse a type which might be being mutated by a Rust thread.
+    if class_object.check_threadsafe().is_err()
+        || class_object.borrow_checker().try_borrow().is_err()
+    {
+        return 0;
+    }
 
-        match catch_unwind(AssertUnwindSafe(move || impl_(instance, visit))) {
-            Ok(Ok(())) => 0,
-            Ok(Err(traverse_error)) => traverse_error.into_inner(),
-            Err(_err) => -1,
+    struct TraverseGuard<'a, T: PyClassImpl>(&'a T::Layout);
+    impl<T: PyClassImpl> Drop for TraverseGuard<'_, T> {
+        fn drop(&mut self) {
+            self.0.borrow_checker().release_borrow()
         }
-    } else {
-        0
+    }
+
+    // `.try_borrow()` above created a borrow, we need to release it when we're done
+    // traversing the object. This allows us to read `instance` safely.
+    let _guard = TraverseGuard::<T>(class_object);
+    let instance = unsafe { &*class_object.contents().value.get() };
+
+    let visit = PyVisit {
+        visit,
+        arg,
+        _guard: PhantomData,
     };
 
-    // Drop lock before trap just in case dropping lock panics
-    drop(lock);
-    trap.disarm();
-    retval
+    match catch_unwind(AssertUnwindSafe(move || impl_(instance, visit))) {
+        Ok(Ok(())) => 0,
+        Ok(Err(traverse_error)) => traverse_error.into_inner(),
+        Err(_err) => -1,
+    }
 }
 
 /// Call super-type traverse method, if necessary.
@@ -464,23 +500,65 @@ unsafe fn call_super_traverse(
 }
 
 /// Calls an implementation of __clear__ for tp_clear
-pub unsafe fn _call_clear(
+pub unsafe fn _call_clear<T>(
     slf: *mut ffi::PyObject,
     impl_: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<()>,
     current_clear: ffi::inquiry,
-) -> c_int {
+) -> c_int
+where
+    T: PyClass,
+{
     unsafe {
         trampoline::trampoline(move |py| {
             let super_retval = call_super_clear(py, slf, current_clear);
             if super_retval != 0 {
                 return Err(PyErr::fetch(py));
             }
             impl_(py, slf)?;
+
+            // Clear the `__dict__`, breaking any reference cycle through the instance's
+            // attributes.
+            //
+            // SAFETY: `slf` is a valid instance of `T`. A shared reference suffices: clearing
+            // the `__dict__` never touches the pyclass data, so needs no borrow check.
+            let class_object: &<T as PyClassImpl>::Layout = &*slf.cast();
+            class_object.contents().dict.clear_dict(py);
+
             Ok(0)
         })
     }
 }
 
+/// `tp_traverse` for a `#[pyclass]` which defines no `__traverse__` of its own: visits the
+/// base type and the instance `__dict__` (if it is a `#[pyclass(dict)]`).
+pub unsafe extern "C" fn synthesized_traverse<T>(
+    slf: *mut ffi::PyObject,
+    visit: ffi::visitproc,
+    arg: *mut c_void,
+) -> c_int
+where
+    T: PyClass,
+{
+    let super_retval = unsafe { call_super_traverse(slf, visit, arg, synthesized_traverse::<T>) };
+    if super_retval != 0 {
+        return super_retval;
+    }
+
+    // SAFETY: `slf` is a valid pointer to an instance of `T`, and traversal is running so no
+    // mutations can occur. The `__dict__` is not Rust data, so needs no thread or borrow check.
+    let class_object: &<T as PyClassImpl>::Layout = unsafe { &*slf.cast() };
+    unsafe { class_object.contents().dict.traverse_dict(visit, arg) }
+}
+
+/// `tp_clear` for a `#[pyclass]` which defines no `__clear__` of its own: calls the base type
+/// and clears the instance `__dict__` (if it is a `#[pyclass(dict)]`).
+pub unsafe extern "C" fn synthesized_clear<T>(slf: *mut ffi::PyObject) -> c_int
+where
+    T: PyClass,
+{
+    unsafe { _call_clear::<T>(slf, |_, _| Ok(()), synthesized_clear::<T>) }
+}
+
 /// Call super-type traverse method, if necessary.
 ///
 /// Adapted from <https://github.com/cython/cython/blob/7acfb375fb54a033f021b0982a3cd40c34fb22ac/Cython/Utility/ExtensionTypes.c#L386>

src/pyclass/create_type_object.rs

@@ -16,7 +16,10 @@ use crate::{
             assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc,
             tp_dealloc_with_gc, PyClassImpl, PyClassItemsIter, PyObjectOffset,
         },
-        pymethods::{_call_clear, Getter, PyGetterDef, PyMethodDefType, PySetterDef, Setter},
+        pymethods::{
+            synthesized_clear, synthesized_traverse, Getter, PyGetterDef, PyMethodDefType,
+            PySetterDef, Setter,
+        },
         trampoline::trampoline,
     },
     pycell::impl_::PyClassObjectLayout,
@@ -51,6 +54,8 @@ where
         base: *mut ffi::PyTypeObject,
         dealloc: unsafe extern "C" fn(*mut ffi::PyObject),
         dealloc_with_gc: unsafe extern "C" fn(*mut ffi::PyObject),
+        synthesized_traverse: ffi::traverseproc,
+        synthesized_clear: ffi::inquiry,
         is_mapping: bool,
         is_sequence: bool,
         is_immutable_type: bool,
@@ -74,6 +79,8 @@ where
                 tp_base: base,
                 tp_dealloc: dealloc,
                 tp_dealloc_with_gc: dealloc_with_gc,
+                synthesized_traverse,
+                synthesized_clear,
                 is_mapping,
                 is_sequence,
                 is_immutable_type,
@@ -100,6 +107,8 @@ where
             T::BaseType::type_object_raw(py),
             tp_dealloc::<T>,
             tp_dealloc_with_gc::<T>,
+            synthesized_traverse::<T>,
+            synthesized_clear::<T>,
             T::IS_MAPPING,
             T::IS_SEQUENCE,
             T::IS_IMMUTABLE_TYPE,
@@ -131,6 +140,10 @@ struct PyTypeBuilder {
     tp_base: *mut ffi::PyTypeObject,
     tp_dealloc: ffi::destructor,
     tp_dealloc_with_gc: ffi::destructor,
+    /// `tp_traverse` / `tp_clear` to install when the class needs the slot but defines no
+    /// `__traverse__` / `__clear__` of its own.
+    synthesized_traverse: ffi::traverseproc,
+    synthesized_clear: ffi::inquiry,
     is_mapping: bool,
     is_sequence: bool,
     is_immutable_type: bool,
@@ -422,6 +435,25 @@ impl PyTypeBuilder {
             }
         }
 
+        // A reference cycle can run through the instance `__dict__` (`obj.x = obj`), so a class
+        // with a `__dict__` must be a GC type. `_call_traverse` / `_call_clear` service the
+        // `__dict__` when the user defines `__traverse__` / `__clear__`; synthesize whichever
+        // slot they did not.
+        //
+        // Must run before the `tp_dealloc` selection below, which keys off `has_traverse`.
+        if self.dict_offset.is_some() {
+            if !self.has_traverse {
+                let synthesized_traverse = self.synthesized_traverse;
+                // Safety: This is the correct slot type for Py_tp_traverse
+                unsafe { self.push_slot(ffi::Py_tp_traverse, synthesized_traverse as *mut c_void) }
+            }
+            if !self.has_clear {
+                let synthesized_clear = self.synthesized_clear;
+                // Safety: This is the correct slot type for Py_tp_clear
+                unsafe { self.push_slot(ffi::Py_tp_clear, synthesized_clear as *mut c_void) }
+            }
+        }
+
         let base_is_gc = unsafe { ffi::PyType_IS_GC(self.tp_base) == 1 };
         let tp_dealloc = if self.has_traverse || base_is_gc {
             self.tp_dealloc_with_gc
@@ -447,8 +479,9 @@ impl PyTypeBuilder {
             assert!(self.has_traverse); // Py_TPFLAGS_HAVE_GC is set when a `__traverse__` method is found
 
             if !self.has_clear {
+                let synthesized_clear = self.synthesized_clear;
                 // Safety: This is the correct slot type for Py_tp_clear
-                unsafe { self.push_slot(ffi::Py_tp_clear, call_super_clear as *mut c_void) }
+                unsafe { self.push_slot(ffi::Py_tp_clear, synthesized_clear as *mut c_void) }
             }
         }
 
@@ -587,10 +620,6 @@ unsafe extern "C" fn no_constructor_defined(
     }
 }
 
-unsafe extern "C" fn call_super_clear(slf: *mut ffi::PyObject) -> c_int {
-    unsafe { _call_clear(slf, |_, _| Ok(()), call_super_clear) }
-}
-
 #[derive(Default)]
 struct GetSetDefBuilder {
     doc: Option<&'static CStr>,