From defa5203b14abb77456047a60fa83b47c7c9821a Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 7 Oct 2024 21:31:48 +0100 Subject: [PATCH] avoid calling `PyType_GetSlot` on static types before Python 3.10 (#4599) * avoid calling `PyType_GetSlot` on static types before Python 3.10 * use the correct tp_free * fixup --- newsfragments/4599.fixed.md | 1 + src/internal.rs | 3 + src/internal/get_slot.rs | 234 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/pycell/impl_.rs | 27 +++-- src/pyclass_init.rs | 17 ++- src/type_object.rs | 29 ----- src/types/any.rs | 26 ++-- 8 files changed, 279 insertions(+), 59 deletions(-) create mode 100644 newsfragments/4599.fixed.md create mode 100644 src/internal.rs create mode 100644 src/internal/get_slot.rs diff --git a/newsfragments/4599.fixed.md b/newsfragments/4599.fixed.md new file mode 100644 index 00000000000..6ed2dea323b --- /dev/null +++ b/newsfragments/4599.fixed.md @@ -0,0 +1 @@ +Fix crash calling `PyType_GetSlot` on static types before Python 3.10. diff --git a/src/internal.rs b/src/internal.rs new file mode 100644 index 00000000000..d42cc2b87fc --- /dev/null +++ b/src/internal.rs @@ -0,0 +1,3 @@ +//! Holding place for code which is not intended to be reachable from outside of PyO3. + +pub(crate) mod get_slot; diff --git a/src/internal/get_slot.rs b/src/internal/get_slot.rs new file mode 100644 index 00000000000..c151e855a14 --- /dev/null +++ b/src/internal/get_slot.rs @@ -0,0 +1,234 @@ +use crate::{ + ffi, + types::{PyType, PyTypeMethods}, + Borrowed, Bound, +}; +use std::os::raw::c_int; + +impl Bound<'_, PyType> { + #[inline] + pub(crate) fn get_slot(&self, slot: Slot) -> as GetSlotImpl>::Type + where + Slot: GetSlotImpl, + { + slot.get_slot(self.as_borrowed()) + } +} + +impl Borrowed<'_, '_, PyType> { + #[inline] + pub(crate) fn get_slot(self, slot: Slot) -> as GetSlotImpl>::Type + where + Slot: GetSlotImpl, + { + slot.get_slot(self) + } +} + +pub(crate) trait GetSlotImpl { + type Type; + fn get_slot(self, tp: Borrowed<'_, '_, PyType>) -> Self::Type; +} + +#[derive(Copy, Clone)] +pub(crate) struct Slot; + +macro_rules! impl_slots { + ($($name:ident: ($slot:ident, $field:ident) -> $tp:ty),+ $(,)?) => { + $( + pub (crate) const $name: Slot<{ ffi::$slot }> = Slot; + + impl GetSlotImpl for Slot<{ ffi::$slot }> { + type Type = $tp; + + #[inline] + fn get_slot(self, tp: Borrowed<'_, '_, PyType>) -> Self::Type { + let ptr = tp.as_type_ptr(); + + #[cfg(not(Py_LIMITED_API))] + unsafe { + (*ptr).$field + } + + #[cfg(Py_LIMITED_API)] + { + #[cfg(not(Py_3_10))] + { + // Calling PyType_GetSlot on static types is not valid before Python 3.10 + // ... so the workaround is to first do a runtime check for these versions + // (3.7, 3.8, 3.9) and then look in the type object anyway. This is only ok + // because we know that the interpreter is not going to change the size + // of the type objects for these historical versions. + if !is_runtime_3_10(tp.py()) + && unsafe { ffi::PyType_HasFeature(ptr, ffi::Py_TPFLAGS_HEAPTYPE) } == 0 + { + return unsafe { (*ptr.cast::()).$field }; + } + } + + // SAFETY: slot type is set carefully to be valid + unsafe { std::mem::transmute(ffi::PyType_GetSlot(ptr, ffi::$slot)) } + } + } + } + )* + }; +} + +// Slots are implemented on-demand as needed. +impl_slots! { + TP_ALLOC: (Py_tp_alloc, tp_alloc) -> Option, + TP_DESCR_GET: (Py_tp_descr_get, tp_descr_get) -> Option, + TP_FREE: (Py_tp_free, tp_free) -> Option, +} + +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +fn is_runtime_3_10(py: crate::Python<'_>) -> bool { + use crate::sync::GILOnceCell; + + static IS_RUNTIME_3_10: GILOnceCell = GILOnceCell::new(); + *IS_RUNTIME_3_10.get_or_init(py, || py.version_info() >= (3, 10)) +} + +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +pub struct PyNumberMethods39Snapshot { + pub nb_add: Option, + pub nb_subtract: Option, + pub nb_multiply: Option, + pub nb_remainder: Option, + pub nb_divmod: Option, + pub nb_power: Option, + pub nb_negative: Option, + pub nb_positive: Option, + pub nb_absolute: Option, + pub nb_bool: Option, + pub nb_invert: Option, + pub nb_lshift: Option, + pub nb_rshift: Option, + pub nb_and: Option, + pub nb_xor: Option, + pub nb_or: Option, + pub nb_int: Option, + pub nb_reserved: *mut std::os::raw::c_void, + pub nb_float: Option, + pub nb_inplace_add: Option, + pub nb_inplace_subtract: Option, + pub nb_inplace_multiply: Option, + pub nb_inplace_remainder: Option, + pub nb_inplace_power: Option, + pub nb_inplace_lshift: Option, + pub nb_inplace_rshift: Option, + pub nb_inplace_and: Option, + pub nb_inplace_xor: Option, + pub nb_inplace_or: Option, + pub nb_floor_divide: Option, + pub nb_true_divide: Option, + pub nb_inplace_floor_divide: Option, + pub nb_inplace_true_divide: Option, + pub nb_index: Option, + pub nb_matrix_multiply: Option, + pub nb_inplace_matrix_multiply: Option, +} + +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +pub struct PySequenceMethods39Snapshot { + pub sq_length: Option, + pub sq_concat: Option, + pub sq_repeat: Option, + pub sq_item: Option, + pub was_sq_slice: *mut std::os::raw::c_void, + pub sq_ass_item: Option, + pub was_sq_ass_slice: *mut std::os::raw::c_void, + pub sq_contains: Option, + pub sq_inplace_concat: Option, + pub sq_inplace_repeat: Option, +} + +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +pub struct PyMappingMethods39Snapshot { + pub mp_length: Option, + pub mp_subscript: Option, + pub mp_ass_subscript: Option, +} + +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +pub struct PyAsyncMethods39Snapshot { + pub am_await: Option, + pub am_aiter: Option, + pub am_anext: Option, +} + +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +pub struct PyBufferProcs39Snapshot { + // not available in limited api, but structure needs to have the right size + pub bf_getbuffer: *mut std::os::raw::c_void, + pub bf_releasebuffer: *mut std::os::raw::c_void, +} + +/// Snapshot of the structure of PyTypeObject for Python 3.7 through 3.9. +/// +/// This is used as a fallback for static types in abi3 when the Python version is less than 3.10; +/// this is a bit of a hack but there's no better option and the structure of the type object is +/// not going to change for those historical versions. +#[repr(C)] +#[cfg(all(Py_LIMITED_API, not(Py_3_10)))] +struct PyTypeObject39Snapshot { + pub ob_base: ffi::PyVarObject, + pub tp_name: *const std::os::raw::c_char, + pub tp_basicsize: ffi::Py_ssize_t, + pub tp_itemsize: ffi::Py_ssize_t, + pub tp_dealloc: Option, + #[cfg(not(Py_3_8))] + pub tp_print: *mut std::os::raw::c_void, // stubbed out, not available in limited API + #[cfg(Py_3_8)] + pub tp_vectorcall_offset: ffi::Py_ssize_t, + pub tp_getattr: Option, + pub tp_setattr: Option, + pub tp_as_async: *mut PyAsyncMethods39Snapshot, + pub tp_repr: Option, + pub tp_as_number: *mut PyNumberMethods39Snapshot, + pub tp_as_sequence: *mut PySequenceMethods39Snapshot, + pub tp_as_mapping: *mut PyMappingMethods39Snapshot, + pub tp_hash: Option, + pub tp_call: Option, + pub tp_str: Option, + pub tp_getattro: Option, + pub tp_setattro: Option, + pub tp_as_buffer: *mut PyBufferProcs39Snapshot, + pub tp_flags: std::os::raw::c_ulong, + pub tp_doc: *const std::os::raw::c_char, + pub tp_traverse: Option, + pub tp_clear: Option, + pub tp_richcompare: Option, + pub tp_weaklistoffset: ffi::Py_ssize_t, + pub tp_iter: Option, + pub tp_iternext: Option, + pub tp_methods: *mut ffi::PyMethodDef, + pub tp_members: *mut ffi::PyMemberDef, + pub tp_getset: *mut ffi::PyGetSetDef, + pub tp_base: *mut ffi::PyTypeObject, + pub tp_dict: *mut ffi::PyObject, + pub tp_descr_get: Option, + pub tp_descr_set: Option, + pub tp_dictoffset: ffi::Py_ssize_t, + pub tp_init: Option, + pub tp_alloc: Option, + pub tp_new: Option, + pub tp_free: Option, + pub tp_is_gc: Option, + pub tp_bases: *mut ffi::PyObject, + pub tp_mro: *mut ffi::PyObject, + pub tp_cache: *mut ffi::PyObject, + pub tp_subclasses: *mut ffi::PyObject, + pub tp_weaklist: *mut ffi::PyObject, + pub tp_del: Option, + pub tp_version_tag: std::os::raw::c_uint, + pub tp_finalize: Option, + #[cfg(Py_3_8)] + pub tp_vectorcall: Option, +} diff --git a/src/lib.rs b/src/lib.rs index 71cdbcea5bd..003a66da7db 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -437,6 +437,7 @@ mod tests; #[macro_use] mod internal_tricks; +mod internal; pub mod buffer; #[doc(hidden)] diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs index efc057e74f7..8fffa976956 100644 --- a/src/pycell/impl_.rs +++ b/src/pycell/impl_.rs @@ -8,7 +8,9 @@ use std::mem::ManuallyDrop; use crate::impl_::pyclass::{ PyClassBaseType, PyClassDict, PyClassImpl, PyClassThreadChecker, PyClassWeakRef, }; -use crate::type_object::{get_tp_free, PyLayout, PySizedLayout}; +use crate::internal::get_slot::TP_FREE; +use crate::type_object::{PyLayout, PySizedLayout}; +use crate::types::{PyType, PyTypeMethods}; use crate::{ffi, PyClass, PyTypeInfo, Python}; use super::{PyBorrowError, PyBorrowMutError}; @@ -221,26 +223,37 @@ where Ok(()) } unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) { - let type_obj = T::type_object_raw(py); + // FIXME: there is potentially subtle issues here if the base is overwritten + // at runtime? To be investigated. + let type_obj = T::type_object(py); + let type_ptr = type_obj.as_type_ptr(); + let actual_type = PyType::from_borrowed_type_ptr(py, ffi::Py_TYPE(slf)); + // For `#[pyclass]` types which inherit from PyAny, we can just call tp_free - if type_obj == std::ptr::addr_of_mut!(ffi::PyBaseObject_Type) { - return get_tp_free(ffi::Py_TYPE(slf))(slf.cast()); + if type_ptr == std::ptr::addr_of_mut!(ffi::PyBaseObject_Type) { + let tp_free = actual_type + .get_slot(TP_FREE) + .expect("PyBaseObject_Type should have tp_free"); + return tp_free(slf.cast()); } // More complex native types (e.g. `extends=PyDict`) require calling the base's dealloc. #[cfg(not(Py_LIMITED_API))] { - if let Some(dealloc) = (*type_obj).tp_dealloc { + // FIXME: should this be using actual_type.tp_dealloc? + if let Some(dealloc) = (*type_ptr).tp_dealloc { // Before CPython 3.11 BaseException_dealloc would use Py_GC_UNTRACK which // assumes the exception is currently GC tracked, so we have to re-track // before calling the dealloc so that it can safely call Py_GC_UNTRACK. #[cfg(not(any(Py_3_11, PyPy)))] - if ffi::PyType_FastSubclass(type_obj, ffi::Py_TPFLAGS_BASE_EXC_SUBCLASS) == 1 { + if ffi::PyType_FastSubclass(type_ptr, ffi::Py_TPFLAGS_BASE_EXC_SUBCLASS) == 1 { ffi::PyObject_GC_Track(slf.cast()); } dealloc(slf); } else { - get_tp_free(ffi::Py_TYPE(slf))(slf.cast()); + (*actual_type.as_type_ptr()) + .tp_free + .expect("type missing tp_free")(slf.cast()); } } diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index 9cd9ff8b1b8..78a044a8c88 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -2,12 +2,13 @@ use crate::callback::IntoPyCallbackOutput; use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::{PyClassBaseType, PyClassDict, PyClassThreadChecker, PyClassWeakRef}; -use crate::types::PyAnyMethods; -use crate::{ffi, Bound, Py, PyClass, PyErr, PyResult, Python}; +use crate::internal::get_slot::TP_ALLOC; +use crate::types::{PyAnyMethods, PyType}; +use crate::{ffi, Borrowed, Bound, Py, PyClass, PyErr, PyResult, Python}; use crate::{ ffi::PyTypeObject, pycell::impl_::{PyClassBorrowChecker, PyClassMutability, PyClassObjectContents}, - type_object::{get_tp_alloc, PyTypeInfo}, + type_object::PyTypeInfo, }; use std::{ cell::UnsafeCell, @@ -50,8 +51,16 @@ impl PyObjectInit for PyNativeTypeInitializer { ) -> PyResult<*mut ffi::PyObject> { // HACK (due to FIXME below): PyBaseObject_Type's tp_new isn't happy with NULL arguments let is_base_object = type_object == std::ptr::addr_of_mut!(ffi::PyBaseObject_Type); + let subtype_borrowed: Borrowed<'_, '_, PyType> = subtype + .cast::() + .assume_borrowed_unchecked(py) + .downcast_unchecked(); + if is_base_object { - let alloc = get_tp_alloc(subtype).unwrap_or(ffi::PyType_GenericAlloc); + let alloc = subtype_borrowed + .get_slot(TP_ALLOC) + .unwrap_or(ffi::PyType_GenericAlloc); + let obj = alloc(subtype, 0); return if obj.is_null() { Err(PyErr::fetch(py)) diff --git a/src/type_object.rs b/src/type_object.rs index c2604be47ff..1e8313ff69b 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -227,32 +227,3 @@ where T::is_type_of_bound(object) } } - -#[inline] -pub(crate) unsafe fn get_tp_alloc(tp: *mut ffi::PyTypeObject) -> Option { - #[cfg(not(Py_LIMITED_API))] - { - (*tp).tp_alloc - } - - #[cfg(Py_LIMITED_API)] - { - let ptr = ffi::PyType_GetSlot(tp, ffi::Py_tp_alloc); - std::mem::transmute(ptr) - } -} - -#[inline] -pub(crate) unsafe fn get_tp_free(tp: *mut ffi::PyTypeObject) -> ffi::freefunc { - #[cfg(not(Py_LIMITED_API))] - { - (*tp).tp_free.unwrap() - } - - #[cfg(Py_LIMITED_API)] - { - let ptr = ffi::PyType_GetSlot(tp, ffi::Py_tp_free); - debug_assert_ne!(ptr, std::ptr::null_mut()); - std::mem::transmute(ptr) - } -} diff --git a/src/types/any.rs b/src/types/any.rs index 50ffebb34db..ad7120418f3 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -4,6 +4,7 @@ use crate::err::{DowncastError, DowncastIntoError, PyErr, PyResult}; use crate::exceptions::{PyAttributeError, PyTypeError}; use crate::ffi_ptr_ext::FfiPtrExt; use crate::instance::Bound; +use crate::internal::get_slot::TP_DESCR_GET; use crate::internal_tricks::ptr_from_ref; use crate::py_result_ext::PyResultExt; use crate::type_object::{PyTypeCheck, PyTypeInfo}; @@ -2360,27 +2361,14 @@ impl<'py> Bound<'py, PyAny> { return Ok(None); }; - // Manually resolve descriptor protocol. - if cfg!(Py_3_10) - || unsafe { ffi::PyType_HasFeature(attr.get_type_ptr(), ffi::Py_TPFLAGS_HEAPTYPE) } != 0 - { - // This is the preferred faster path, but does not work on static types (generally, - // types defined in extension modules) before Python 3.10. + // Manually resolve descriptor protocol. (Faster than going through Python.) + if let Some(descr_get) = attr.get_type().get_slot(TP_DESCR_GET) { + // attribute is a descriptor, resolve it unsafe { - let descr_get_ptr = ffi::PyType_GetSlot(attr.get_type_ptr(), ffi::Py_tp_descr_get); - if descr_get_ptr.is_null() { - return Ok(Some(attr)); - } - let descr_get: ffi::descrgetfunc = std::mem::transmute(descr_get_ptr); - let ret = descr_get(attr.as_ptr(), self.as_ptr(), self_type.as_ptr()); - ret.assume_owned_or_err(py).map(Some) + descr_get(attr.as_ptr(), self.as_ptr(), self_type.as_ptr()) + .assume_owned_or_err(py) + .map(Some) } - } else if let Ok(descr_get) = attr - .get_type() - .as_borrowed() - .getattr(crate::intern!(py, "__get__")) - { - descr_get.call1((attr, self, self_type)).map(Some) } else { Ok(Some(attr)) }