fix cases of incorrect allocator or deallocator inheritance

newsfragments/6559.fixed.1.md

@@ -0,0 +1 @@
+Fix crash when subclassing a `#[pyclass(freelist)]`.

newsfragments/6559.fixed.md

@@ -0,0 +1 @@
+Fix crash for pyclasses which inherit native types with a custom allocator which doesn't allocate space for the `#[pyclass]` data.

pyo3-macros-backend/src/pyclass.rs

@@ -2829,6 +2829,13 @@ impl<'a> PyClassImplsBuilder<'a> {
         let is_immutable_type =
             is_immutable_type.then(|| quote! { const IS_IMMUTABLE_TYPE: bool = true; });
 
+        let allocators = self.attr.options.freelist.is_some().then(|| {
+            quote! {
+                const TP_ALLOC: #pyo3_path::ffi::allocfunc = #pyo3_path::impl_::pyclass::alloc_with_freelist::<Self> as _;
+                const TP_FREE: #pyo3_path::ffi::freefunc = #pyo3_path::impl_::pyclass::free_with_freelist::<Self> as _;
+            }
+        });
+
         let dict_offset = if self.attr.options.dict.is_some() {
             quote! {
                 fn dict_offset() -> ::core::option::Option<#pyo3_path::impl_::pyclass::PyObjectOffset> {
@@ -2916,14 +2923,10 @@ impl<'a> PyClassImplsBuilder<'a> {
             .iter()
             .map(|slot| &slot.slot_def)
             .collect::<Vec<_>>();
-        let freelist_slots = self.freelist_slots(ctx);
 
         // When there are no intrinsic items, point at a shared empty `PyClassItems` to keep the
         // generated code small.
-        let items_iter = if default_method_defs.is_empty()
-            && default_slot_defs.is_empty()
-            && freelist_slots.is_empty()
-        {
+        let items_iter = if default_method_defs.is_empty() && default_slot_defs.is_empty() {
             quote! {
                 fn items_iter() -> #pyo3_path::impl_::pyclass::PyClassItemsIter {
                     #pyo3_path::impl_::pyclass::PyClassItemsIter::new(
@@ -2937,7 +2940,7 @@ impl<'a> PyClassImplsBuilder<'a> {
                 fn items_iter() -> #pyo3_path::impl_::pyclass::PyClassItemsIter {
                     static INTRINSIC_ITEMS: #pyo3_path::impl_::pyclass::PyClassItems = #pyo3_path::impl_::pyclass::PyClassItems {
                         methods: &[#(#default_method_defs),*],
-                        slots: &[#(#default_slot_defs),* #(#freelist_slots),*],
+                        slots: &[#(#default_slot_defs),*],
                     };
                     #pyo3_path::impl_::pyclass::PyClassItemsIter::new(&INTRINSIC_ITEMS, #pymethods_items)
                 }
@@ -3059,6 +3062,8 @@ impl<'a> PyClassImplsBuilder<'a> {
                 #is_sequence
                 #is_immutable_type
 
+                #allocators
+
                 type Layout = <Self::BaseNativeType as #pyo3_path::impl_::pyclass::PyClassBaseType>::Layout<Self>;
                 type BaseType = #base;
                 type ThreadChecker = #thread_checker;
@@ -3136,30 +3141,6 @@ impl<'a> PyClassImplsBuilder<'a> {
         })
     }
 
-    fn freelist_slots(&self, ctx: &Ctx) -> Vec<TokenStream> {
-        let Ctx { pyo3_path, .. } = ctx;
-        let cls = self.cls_ident;
-
-        if self.attr.options.freelist.is_some() {
-            vec![
-                quote! {
-                    #pyo3_path::ffi::PyType_Slot {
-                        slot: #pyo3_path::ffi::Py_tp_alloc,
-                        pfunc: #pyo3_path::impl_::pyclass::alloc_with_freelist::<#cls> as *mut _,
-                    }
-                },
-                quote! {
-                    #pyo3_path::ffi::PyType_Slot {
-                        slot: #pyo3_path::ffi::Py_tp_free,
-                        pfunc: #pyo3_path::impl_::pyclass::free_with_freelist::<#cls> as *mut _,
-                    }
-                },
-            ]
-        } else {
-            Vec::new()
-        }
-    }
-
     #[cfg(feature = "experimental-inspect")]
     fn impl_introspection(&self, ctx: &Ctx, parent: Option<&Ident>) -> TokenStream {
         let Ctx { pyo3_path, .. } = ctx;

src/impl_/pyclass.rs

@@ -228,6 +228,12 @@ pub trait PyClassImpl: Sized + 'static {
     /// #[pyclass(immutable_type)]
     const IS_IMMUTABLE_TYPE: bool = false;
 
+    /// Implementation of `tp_alloc`. Must pair with `TP_FREE`.
+    const TP_ALLOC: ffi::allocfunc = ffi::PyType_GenericAlloc;
+
+    /// Implementation of `tp_free`. Must pair with `TP_ALLOC`.
+    const TP_FREE: ffi::freefunc = ffi::PyObject_GC_Del;
+
     /// Description of how this class is laid out in memory
     type Layout: PyClassObjectLayout<Self>;
 

src/pyclass/create_type_object.rs

@@ -48,14 +48,25 @@ pub(crate) fn create_type_object<T>(py: Python<'_>) -> PyResult<PyClassTypeObjec
 where
     T: PyClass,
 {
-    // Written this way to monomorphize the majority of the logic.
+    /// Slots which are implemented by PyO3 in all cases because the inherited implementations
+    /// are not suitable for `#[pyclass]` internals.
+    struct FixedSlots {
+        // See https://github.com/python/cpython/issues/160749 - explicitly set tp_alloc in all cases
+        // to ensure that the allocator is suitable for the type. We pair it with `tp_free`.
+        alloc: ffi::allocfunc,
+        free: ffi::freefunc,
+        // tp_dealloc, tp_traverse and tp_clear need to ensure correct handling of the GC
+        dealloc: ffi::destructor,
+        traverse: ffi::traverseproc,
+        synthesized_clear: ffi::inquiry,
+    }
+
+    // Written this way to avoid monomorphizing the majority of the logic.
     #[expect(clippy::too_many_arguments)]
     unsafe fn inner(
         py: Python<'_>,
         base: *mut ffi::PyTypeObject,
-        tp_dealloc: ffi::destructor,
-        tp_traverse: ffi::traverseproc,
-        synthesized_clear: ffi::inquiry,
+        fixed_slots: &FixedSlots,
         is_mapping: bool,
         is_sequence: bool,
         is_immutable_type: bool,
@@ -70,16 +81,32 @@ where
     ) -> PyResult<PyClassTypeObject> {
         unsafe {
             PyTypeBuilder {
-                slots: Vec::new(),
+                slots: vec![
+                    ffi::PyType_Slot {
+                        slot: ffi::Py_tp_alloc,
+                        pfunc: fixed_slots.alloc as *mut c_void,
+                    },
+                    ffi::PyType_Slot {
+                        slot: ffi::Py_tp_free,
+                        pfunc: fixed_slots.free as *mut c_void,
+                    },
+                    ffi::PyType_Slot {
+                        slot: ffi::Py_tp_dealloc,
+                        pfunc: fixed_slots.dealloc as *mut c_void,
+                    },
+                    ffi::PyType_Slot {
+                        slot: ffi::Py_tp_traverse,
+                        pfunc: fixed_slots.traverse as *mut c_void,
+                    },
+                    // NB there is logic inside `PyTypeBuilder::build` to install a `tp_clear` only when necessary
+                ],
                 method_defs: Vec::new(),
                 member_defs: Vec::new(),
                 getset_builders: HashMap::new(),
                 #[cfg(all(not(Py_LIMITED_API), not(Py_3_10)))]
                 cleanup: Vec::new(),
                 tp_base: base,
-                tp_dealloc,
-                tp_traverse,
-                synthesized_clear,
+                synthesized_clear: fixed_slots.synthesized_clear,
                 is_mapping,
                 is_sequence,
                 is_immutable_type,
@@ -102,9 +129,13 @@ where
         inner(
             py,
             T::BaseType::type_object_raw(py),
-            tp_dealloc::<T>,
-            tp_traverse::<T>,
-            synthesized_clear::<T>,
+            &FixedSlots {
+                alloc: T::TP_ALLOC,
+                free: T::TP_FREE,
+                dealloc: tp_dealloc::<T>,
+                traverse: tp_traverse::<T>,
+                synthesized_clear: synthesized_clear::<T>,
+            },
             T::IS_MAPPING,
             T::IS_SEQUENCE,
             T::IS_IMMUTABLE_TYPE,
@@ -134,8 +165,6 @@ struct PyTypeBuilder {
     #[cfg(all(not(Py_LIMITED_API), not(Py_3_10)))]
     cleanup: Vec<PyTypeBuilderCleanup>,
     tp_base: *mut ffi::PyTypeObject,
-    tp_dealloc: ffi::destructor,
-    tp_traverse: ffi::traverseproc,
     /// `tp_clear` to install when the class defines no `__clear__` of its own
     synthesized_clear: ffi::inquiry,
     is_mapping: bool,
@@ -420,9 +449,6 @@ impl PyTypeBuilder {
             }
         }
 
-        unsafe { self.push_slot(ffi::Py_tp_dealloc, self.tp_dealloc as *mut c_void) }
-        unsafe { self.push_slot(ffi::Py_tp_traverse, self.tp_traverse as *mut c_void) }
-
         // We may need to install a `tp_clear` if this type didn't define one:
         if !self.has_clear {
             if self.dict_offset.is_some() {

tests/test_inheritance.rs

@@ -233,6 +233,21 @@ except Exception as e:
     });
 }
 
+/// Children of freelist types will not be able to use the freelist, but they should drop cleanly.
+#[test]
+fn inherit_freelist() {
+    #[pyclass(freelist = 2, subclass)]
+    struct Base;
+
+    #[pyclass(extends=Base)]
+    struct Child;
+
+    Python::attach(|py| {
+        drop(Bound::new(py, (Child, Base)).unwrap());
+        drop(Bound::new(py, Base).unwrap());
+    });
+}
+
 // Subclassing builtin types is not possible in the LIMITED API before 3.12
 #[cfg(any(not(Py_LIMITED_API), Py_3_12))]
 mod inheriting_native_type {
@@ -334,6 +349,28 @@ mod inheriting_native_type {
         })
     }
 
+    #[cfg(not(any(PyPy, GraalPy)))]
+    #[test]
+    fn inherit_time() {
+        #[pyclass(extends=pyo3::types::PyTime)]
+        struct TimeWithName {
+            #[pyo3(get, name = "name")]
+            _name: &'static str,
+        }
+
+        Python::attach(|py| {
+            let time = Bound::new(py, TimeWithName { _name: "Hello :)" }).unwrap();
+            // NB: due to lack of ability to pass arguments to super new, we can't construct times other
+            // than midnight yet. But the allocation & drop should at least be sound.
+            py_run!(
+                py,
+                time,
+                "assert str(time) == '00:00:00', str(time); assert time.name == 'Hello :)', time.name"
+            );
+            drop(time);
+        });
+    }
+
     #[pyclass(extends=PyException)]
     struct CustomException {
         #[pyo3(get)]