diff --git a/newsfragments/6559.fixed.1.md b/newsfragments/6559.fixed.1.md new file mode 100644 index 00000000000..26f7f2f6c21 --- /dev/null +++ b/newsfragments/6559.fixed.1.md @@ -0,0 +1 @@ +Fix crash when subclassing a `#[pyclass(freelist)]`. diff --git a/newsfragments/6559.fixed.md b/newsfragments/6559.fixed.md new file mode 100644 index 00000000000..6b673e26f7f --- /dev/null +++ b/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. diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 44855a25f7e..dbd3eae71c9 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/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:: as _; + const TP_FREE: #pyo3_path::ffi::freefunc = #pyo3_path::impl_::pyclass::free_with_freelist:: 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::>(); - 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 = ::Layout; type BaseType = #base; type ThreadChecker = #thread_checker; @@ -3136,30 +3141,6 @@ impl<'a> PyClassImplsBuilder<'a> { }) } - fn freelist_slots(&self, ctx: &Ctx) -> Vec { - 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; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index a7723589671..b90f2d3b53d 100644 --- a/src/impl_/pyclass.rs +++ b/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; diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index 464dd26e672..02fe0b13db5 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -48,14 +48,25 @@ pub(crate) fn create_type_object(py: Python<'_>) -> PyResult, 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 { 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::, - tp_traverse::, - synthesized_clear::, + &FixedSlots { + alloc: T::TP_ALLOC, + free: T::TP_FREE, + dealloc: tp_dealloc::, + traverse: tp_traverse::, + synthesized_clear: synthesized_clear::, + }, 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, 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() { diff --git a/tests/test_inheritance.rs b/tests/test_inheritance.rs index 657c207d0de..73636f9eb2a 100644 --- a/tests/test_inheritance.rs +++ b/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)]