From 426b218b8111bc5155433ae4af786241513eeeca Mon Sep 17 00:00:00 2001 From: Matthew Broadway Date: Sun, 3 Nov 2024 17:27:27 +0000 Subject: [PATCH] wrap PyClassObjectContents to prevent it from leaking outside pyo3 --- src/impl_/coroutine.rs | 2 +- src/impl_/pycell.rs | 2 +- src/impl_/pyclass.rs | 10 +- src/impl_/pyclass_init.rs | 8 +- src/impl_/pymethods.rs | 10 +- src/instance.rs | 2 +- src/pycell.rs | 2 +- src/pycell/impl_.rs | 96 ++++++++++++------- src/pyclass/create_type_object.rs | 2 +- src/pyclass_init.rs | 6 +- src/type_object.rs | 2 +- tests/ui/invalid_extend_variable_sized.stderr | 8 +- 12 files changed, 92 insertions(+), 58 deletions(-) diff --git a/src/impl_/coroutine.rs b/src/impl_/coroutine.rs index 0ce3a946ffb..9856ba0b013 100644 --- a/src/impl_/coroutine.rs +++ b/src/impl_/coroutine.rs @@ -6,7 +6,7 @@ use std::{ use crate::{ coroutine::{cancel::ThrowCallback, Coroutine}, instance::Bound, - pycell::impl_::{InternalPyClassObjectLayout, PyClassBorrowChecker}, + pycell::impl_::{PyClassBorrowChecker, PyClassObjectLayout}, pyclass::boolean_struct::False, types::{PyAnyMethods, PyString}, IntoPyObject, Py, PyAny, PyClass, PyErr, PyResult, Python, diff --git a/src/impl_/pycell.rs b/src/impl_/pycell.rs index f9ddd8e8389..850ab1492c5 100644 --- a/src/impl_/pycell.rs +++ b/src/impl_/pycell.rs @@ -1,5 +1,5 @@ //! Externally-accessible implementation of pycell pub use crate::pycell::impl_::{ - GetBorrowChecker, PyClassMutability, PyClassObjectBase, PyClassObjectLayout, + GetBorrowChecker, PyClassMutability, PyClassObjectBase, PyClassObjectBaseLayout, PyStaticClassObject, PyVariableClassObject, PyVariableClassObjectBase, }; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 6fa027047c8..b96be514568 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -6,11 +6,11 @@ use crate::{ ffi, impl_::{ freelist::FreeList, - pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectLayout}, + pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectBaseLayout}, pyclass_init::PyObjectInit, pymethods::{PyGetterDef, PyMethodDefType}, }, - pycell::{impl_::InternalPyClassObjectLayout, PyBorrowError}, + pycell::{impl_::PyClassObjectLayout, PyBorrowError}, types::{any::PyAnyMethods, PyBool}, Borrowed, BoundObject, Py, PyAny, PyClass, PyErr, PyRef, PyResult, PyTypeInfo, Python, }; @@ -170,7 +170,7 @@ pub trait PyClassImpl: Sized + 'static { const IS_SEQUENCE: bool = false; /// Description of how this class is laid out in memory - type Layout: InternalPyClassObjectLayout; + type Layout: PyClassObjectLayout; /// Base class type BaseType: PyTypeInfo + PyClassBaseType; @@ -1137,7 +1137,7 @@ impl PyClassThreadChecker for ThreadCheckerImpl { ) )] pub trait PyClassBaseType: Sized { - type LayoutAsBase: PyClassObjectLayout; + type LayoutAsBase: PyClassObjectBaseLayout; type BaseNativeType; type Initializer: PyObjectInit; type PyClassMutability: PyClassMutability; @@ -1549,7 +1549,7 @@ where let class_ptr = obj.cast::<::Layout>(); // Safety: the object `obj` must have the layout `ClassT::Layout` let class_obj = unsafe { &mut *class_ptr }; - let contents = class_obj.contents_mut() as *mut PyClassObjectContents; + let contents = (&mut class_obj.contents_mut().0) as *mut PyClassObjectContents; (contents.cast::(), offset) } #[cfg(not(Py_3_12))] diff --git a/src/impl_/pyclass_init.rs b/src/impl_/pyclass_init.rs index 7a3e598176b..496bcdb373a 100644 --- a/src/impl_/pyclass_init.rs +++ b/src/impl_/pyclass_init.rs @@ -2,7 +2,9 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::PyClassImpl; use crate::internal::get_slot::TP_ALLOC; -use crate::pycell::impl_::{InternalPyClassObjectLayout, PyClassObjectContents}; +use crate::pycell::impl_::{ + PyClassObjectContents, PyClassObjectLayout, WrappedPyClassObjectContents, +}; use crate::types::PyType; use crate::{ffi, Borrowed, PyErr, PyResult, Python}; use crate::{ffi::PyTypeObject, sealed::Sealed, type_object::PyTypeInfo}; @@ -10,7 +12,9 @@ use std::marker::PhantomData; pub unsafe fn initialize_with_default(obj: *mut ffi::PyObject) { let contents = T::Layout::contents_uninitialised(obj); - (*contents).write(PyClassObjectContents::new(T::default())); + (*contents).write(WrappedPyClassObjectContents(PyClassObjectContents::new( + T::default(), + ))); } /// Initializer for Python types. diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 23d1643fb7c..37632873f04 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -2,9 +2,9 @@ use crate::exceptions::PyStopAsyncIteration; use crate::gil::LockGIL; use crate::impl_::callback::IntoPyCallbackOutput; use crate::impl_::panic::PanicTrap; -use crate::impl_::pycell::PyClassObjectLayout; +use crate::impl_::pycell::PyClassObjectBaseLayout; use crate::internal::get_slot::{get_slot, TP_BASE, TP_CLEAR, TP_TRAVERSE}; -use crate::pycell::impl_::{InternalPyClassObjectLayout, PyClassBorrowChecker as _}; +use crate::pycell::impl_::{PyClassBorrowChecker as _, PyClassObjectLayout}; use crate::pycell::{PyBorrowError, PyBorrowMutError}; use crate::pyclass::boolean_struct::False; use crate::types::any::PyAnyMethods; @@ -310,8 +310,8 @@ where 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, U: PyClassImpl, V: InternalPyClassObjectLayout>(&'a V, PhantomData); - impl> Drop for TraverseGuard<'_, U, V> { + struct TraverseGuard<'a, U: PyClassImpl, V: PyClassObjectLayout>(&'a V, PhantomData); + impl> Drop for TraverseGuard<'_, U, V> { fn drop(&mut self) { self.0.borrow_checker().release_borrow() } @@ -320,7 +320,7 @@ where // `.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(class_object, PhantomData); - let instance = &*class_object.contents().value.get(); + let instance = &*class_object.contents().0.value.get(); let visit = PyVisit { visit, arg, _guard: PhantomData }; diff --git a/src/instance.rs b/src/instance.rs index 79b59be421a..093b8b17fcf 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2,7 +2,7 @@ use crate::conversion::IntoPyObject; use crate::err::{self, PyErr, PyResult}; use crate::impl_::pyclass::PyClassImpl; use crate::internal_tricks::ptr_from_ref; -use crate::pycell::impl_::InternalPyClassObjectLayout; +use crate::pycell::impl_::PyClassObjectLayout; use crate::pycell::{PyBorrowError, PyBorrowMutError}; use crate::pyclass::boolean_struct::{False, True}; use crate::types::{any::PyAnyMethods, string::PyStringMethods, typeobject::PyTypeMethods}; diff --git a/src/pycell.rs b/src/pycell.rs index bbf3804ad01..057b4c2b77f 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -208,7 +208,7 @@ use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; pub(crate) mod impl_; -use impl_::{InternalPyClassObjectLayout, PyClassBorrowChecker, PyClassObjectLayout}; +use impl_::{PyClassBorrowChecker, PyClassObjectBaseLayout, PyClassObjectLayout}; /// A wrapper type for an immutably borrowed value from a [`Bound<'py, T>`]. /// diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs index 96eafc2348c..8bf56c05131 100644 --- a/src/pycell/impl_.rs +++ b/src/pycell/impl_.rs @@ -187,13 +187,13 @@ pub trait GetBorrowChecker { impl> GetBorrowChecker for MutableClass { fn borrow_checker(class_object: &T::Layout) -> &BorrowChecker { - &class_object.contents().borrow_checker + &class_object.contents().0.borrow_checker } } impl> GetBorrowChecker for ImmutableClass { fn borrow_checker(class_object: &T::Layout) -> &EmptySlot { - &class_object.contents().borrow_checker + &class_object.contents().0.borrow_checker } } @@ -226,7 +226,7 @@ pub struct PyVariableClassObjectBase { unsafe impl PyLayout for PyVariableClassObjectBase {} -impl PyClassObjectLayout for PyVariableClassObjectBase { +impl PyClassObjectBaseLayout for PyVariableClassObjectBase { fn ensure_threadsafe(&self) {} fn check_threadsafe(&self) -> Result<(), PyBorrowError> { Ok(()) @@ -237,7 +237,7 @@ impl PyClassObjectLayout for PyVariableClassObjectBase { } #[doc(hidden)] -pub trait PyClassObjectLayout: PyLayout { +pub trait PyClassObjectBaseLayout: PyLayout { fn ensure_threadsafe(&self); fn check_threadsafe(&self) -> Result<(), PyBorrowError>; /// Implementation of tp_dealloc. @@ -247,6 +247,30 @@ pub trait PyClassObjectLayout: PyLayout { unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject); } +/// Allow [PyClassObjectLayout] to have public visibility without leaking the structure of [PyClassObjectContents]. +#[doc(hidden)] +#[repr(transparent)] +pub struct WrappedPyClassObjectContents(pub(crate) PyClassObjectContents); + +impl<'a, T: PyClassImpl> From<&'a PyClassObjectContents> + for &'a WrappedPyClassObjectContents +{ + fn from(value: &'a PyClassObjectContents) -> &'a WrappedPyClassObjectContents { + // Safety: Wrapped struct must use repr(transparent) + unsafe { std::mem::transmute(value) } + } +} + +impl<'a, T: PyClassImpl> From<&'a mut PyClassObjectContents> + for &'a mut WrappedPyClassObjectContents +{ + fn from(value: &'a mut PyClassObjectContents) -> &'a mut WrappedPyClassObjectContents { + // Safety: Wrapped struct must use repr(transparent) + unsafe { std::mem::transmute(value) } + } +} + +/// Functionality required for creating and managing the memory associated with a pyclass annotated struct. #[doc(hidden)] #[cfg_attr( all(diagnostic_namespace), @@ -256,18 +280,18 @@ pub trait PyClassObjectLayout: PyLayout { note = "the python version being built against influences which layouts are valid", ) )] -pub trait InternalPyClassObjectLayout: PyClassObjectLayout { +pub trait PyClassObjectLayout: PyClassObjectBaseLayout { /// Obtain a pointer to the contents of an uninitialized PyObject of this type /// Safety: the provided object must have the layout that the implementation is expecting unsafe fn contents_uninitialised( obj: *mut ffi::PyObject, - ) -> *mut MaybeUninit>; + ) -> *mut MaybeUninit>; fn get_ptr(&self) -> *mut T; - fn contents(&self) -> &PyClassObjectContents; + fn contents(&self) -> &WrappedPyClassObjectContents; - fn contents_mut(&mut self) -> &mut PyClassObjectContents; + fn contents_mut(&mut self) -> &mut WrappedPyClassObjectContents; fn ob_base(&self) -> &::LayoutAsBase; @@ -287,7 +311,7 @@ pub trait InternalPyClassObjectLayout: PyClassObjectLayout { fn borrow_checker(&self) -> &::Checker; } -impl PyClassObjectLayout for PyClassObjectBase +impl PyClassObjectBaseLayout for PyClassObjectBase where U: PySizedLayout, T: PyTypeInfo, @@ -301,6 +325,10 @@ where } } +/// Implementation of tp_dealloc. +/// # Safety +/// - obj must be a valid pointer to an instance of the type at `type_ptr` or a subclass. +/// - obj must not be used after this call (as it will be freed). unsafe fn tp_dealloc(py: Python<'_>, obj: *mut ffi::PyObject, type_ptr: *mut ffi::PyTypeObject) { // FIXME: there is potentially subtle issues here if the base is overwritten // at runtime? To be investigated. @@ -376,14 +404,14 @@ pub struct PyStaticClassObject { contents: PyClassObjectContents, } -impl InternalPyClassObjectLayout for PyStaticClassObject { +impl PyClassObjectLayout for PyStaticClassObject { unsafe fn contents_uninitialised( obj: *mut ffi::PyObject, - ) -> *mut MaybeUninit> { + ) -> *mut MaybeUninit> { #[repr(C)] struct PartiallyInitializedClassObject { _ob_base: ::LayoutAsBase, - contents: MaybeUninit>, + contents: MaybeUninit>, } let obj: *mut PartiallyInitializedClassObject = obj.cast(); addr_of_mut!((*obj).contents) @@ -397,12 +425,12 @@ impl InternalPyClassObjectLayout for PyStaticClassObject { &self.ob_base } - fn contents(&self) -> &PyClassObjectContents { - &self.contents + fn contents(&self) -> &WrappedPyClassObjectContents { + (&self.contents).into() } - fn contents_mut(&mut self) -> &mut PyClassObjectContents { - &mut self.contents + fn contents_mut(&mut self) -> &mut WrappedPyClassObjectContents { + (&mut self.contents).into() } /// used to set PyType_Spec::basicsize @@ -453,9 +481,9 @@ impl InternalPyClassObjectLayout for PyStaticClassObject { unsafe impl PyLayout for PyStaticClassObject {} impl PySizedLayout for PyStaticClassObject {} -impl PyClassObjectLayout for PyStaticClassObject +impl PyClassObjectBaseLayout for PyStaticClassObject where - ::LayoutAsBase: PyClassObjectLayout, + ::LayoutAsBase: PyClassObjectBaseLayout, { fn ensure_threadsafe(&self) { self.contents.thread_checker.ensure(); @@ -470,7 +498,7 @@ where unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) { // Safety: Python only calls tp_dealloc when no references to the object remain. let class_object = &mut *(slf.cast::()); - class_object.contents_mut().dealloc(py, slf); + class_object.contents_mut().0.dealloc(py, slf); ::LayoutAsBase::tp_dealloc(py, slf) } } @@ -482,41 +510,41 @@ pub struct PyVariableClassObject { impl PyVariableClassObject { #[cfg(Py_3_12)] - fn get_contents_of_obj(obj: *mut ffi::PyObject) -> *mut PyClassObjectContents { + fn get_contents_of_obj(obj: *mut ffi::PyObject) -> *mut WrappedPyClassObjectContents { // https://peps.python.org/pep-0697/ let type_obj = unsafe { ffi::Py_TYPE(obj) }; let pointer = unsafe { ffi::PyObject_GetTypeData(obj, type_obj) }; - pointer as *mut PyClassObjectContents + pointer as *mut WrappedPyClassObjectContents } #[cfg(Py_3_12)] - fn get_contents_ptr(&self) -> *mut PyClassObjectContents { + fn get_contents_ptr(&self) -> *mut WrappedPyClassObjectContents { Self::get_contents_of_obj(self as *const PyVariableClassObject as *mut ffi::PyObject) } } #[cfg(Py_3_12)] -impl InternalPyClassObjectLayout for PyVariableClassObject { +impl PyClassObjectLayout for PyVariableClassObject { unsafe fn contents_uninitialised( obj: *mut ffi::PyObject, - ) -> *mut MaybeUninit> { - Self::get_contents_of_obj(obj) as *mut MaybeUninit> + ) -> *mut MaybeUninit> { + Self::get_contents_of_obj(obj) as *mut MaybeUninit> } fn get_ptr(&self) -> *mut T { - self.contents().value.get() + self.contents().0.value.get() } fn ob_base(&self) -> &::LayoutAsBase { &self.ob_base } - fn contents(&self) -> &PyClassObjectContents { - unsafe { (self.get_contents_ptr() as *const PyClassObjectContents).as_ref() } + fn contents(&self) -> &WrappedPyClassObjectContents { + unsafe { self.get_contents_ptr().cast_const().as_ref() } .expect("should be able to cast PyClassObjectContents pointer") } - fn contents_mut(&mut self) -> &mut PyClassObjectContents { + fn contents_mut(&mut self) -> &mut WrappedPyClassObjectContents { unsafe { self.get_contents_ptr().as_mut() } .expect("should be able to cast PyClassObjectContents pointer") } @@ -560,16 +588,16 @@ impl InternalPyClassObjectLayout for PyVariableClassObject unsafe impl PyLayout for PyVariableClassObject {} #[cfg(Py_3_12)] -impl PyClassObjectLayout for PyVariableClassObject +impl PyClassObjectBaseLayout for PyVariableClassObject where - ::LayoutAsBase: PyClassObjectLayout, + ::LayoutAsBase: PyClassObjectBaseLayout, { fn ensure_threadsafe(&self) { - self.contents().thread_checker.ensure(); + self.contents().0.thread_checker.ensure(); self.ob_base.ensure_threadsafe(); } fn check_threadsafe(&self) -> Result<(), PyBorrowError> { - if !self.contents().thread_checker.check() { + if !self.contents().0.thread_checker.check() { return Err(PyBorrowError { _private: () }); } self.ob_base.check_threadsafe() @@ -577,7 +605,7 @@ where unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) { // Safety: Python only calls tp_dealloc when no references to the object remain. let class_object = &mut *(slf.cast::()); - class_object.contents_mut().dealloc(py, slf); + class_object.contents_mut().0.dealloc(py, slf); ::LayoutAsBase::tp_dealloc(py, slf) } } diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index f902f8fffa6..fe0bdbfe5ce 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -11,7 +11,7 @@ use crate::{ trampoline::trampoline, }, internal_tricks::ptr_from_ref, - pycell::impl_::InternalPyClassObjectLayout, + pycell::impl_::PyClassObjectLayout, types::{typeobject::PyTypeMethods, PyType}, Py, PyClass, PyResult, PyTypeInfo, Python, }; diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index df7a2edeb9e..baa1df95b99 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -3,7 +3,7 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::callback::IntoPyCallbackOutput; use crate::impl_::pyclass::{PyClassBaseType, PyClassImpl}; use crate::impl_::pyclass_init::{PyNativeTypeInitializer, PyObjectInit}; -use crate::pycell::impl_::InternalPyClassObjectLayout; +use crate::pycell::impl_::{PyClassObjectLayout, WrappedPyClassObjectContents}; use crate::types::PyAnyMethods; use crate::{ffi, Bound, Py, PyClass, PyResult, Python}; use crate::{ffi::PyTypeObject, pycell::impl_::PyClassObjectContents}; @@ -167,7 +167,9 @@ impl PyClassInitializer { let obj = super_init.into_new_object(py, target_type)?; let contents = ::Layout::contents_uninitialised(obj); - (*contents).write(PyClassObjectContents::new(init)); + (*contents).write(WrappedPyClassObjectContents(PyClassObjectContents::new( + init, + ))); // Safety: obj is a valid pointer to an object of type `target_type`, which` is a known // subclass of `T` diff --git a/src/type_object.rs b/src/type_object.rs index aaa6431db07..0aabcf82a1c 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -44,7 +44,7 @@ pub unsafe trait PyTypeInfo: Sized { const MODULE: Option<&'static str>; /// The type of object layout to use for ancestors or descendants of this type. - /// should implement `InternalPyClassObjectLayout` in order to actually use it as a layout. + /// should implement `PyClassObjectLayout` in order to actually use it as a layout. type Layout; /// Returns the PyTypeObject instance for this type. diff --git a/tests/ui/invalid_extend_variable_sized.stderr b/tests/ui/invalid_extend_variable_sized.stderr index 5acd7e732bf..5f25623d5e7 100644 --- a/tests/ui/invalid_extend_variable_sized.stderr +++ b/tests/ui/invalid_extend_variable_sized.stderr @@ -4,12 +4,12 @@ error[E0277]: the class layout is not valid 4 | #[pyclass(extends=PyType)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ required for `#[pyclass(extends=...)]` | - = help: the trait `pyo3::pycell::impl_::InternalPyClassObjectLayout` is not implemented for `PyVariableClassObject` + = help: the trait `pyo3::pycell::impl_::PyClassObjectLayout` is not implemented for `PyVariableClassObject` = note: the python version being built against influences which layouts are valid - = help: the trait `pyo3::pycell::impl_::InternalPyClassObjectLayout` is implemented for `PyStaticClassObject` + = help: the trait `pyo3::pycell::impl_::PyClassObjectLayout` is implemented for `PyStaticClassObject` note: required by a bound in `PyClassImpl::Layout` --> src/impl_/pyclass.rs | - | type Layout: InternalPyClassObjectLayout; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PyClassImpl::Layout` + | type Layout: PyClassObjectLayout; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PyClassImpl::Layout` = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)