From dd99fb7b5b8c87ef9c1014194d25fd91bc5f0a5a Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 4 Oct 2024 12:24:22 +0100 Subject: [PATCH 1/2] add `Bound::from_owned_ptr_unchecked` (crate-private for now) --- src/ffi_ptr_ext.rs | 15 +++++++++++++++ src/instance.rs | 21 +++++++++++++++++++++ src/pycell.rs | 4 ++-- src/types/dict.rs | 6 +++++- src/types/weakref/anyref.rs | 4 +++- src/types/weakref/proxy.rs | 4 +++- src/types/weakref/reference.rs | 4 +++- 7 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/ffi_ptr_ext.rs b/src/ffi_ptr_ext.rs index 183b0e3734e..956b4e8406c 100644 --- a/src/ffi_ptr_ext.rs +++ b/src/ffi_ptr_ext.rs @@ -6,10 +6,20 @@ use crate::{ }; pub(crate) trait FfiPtrExt: Sealed { + /// Assumes this pointer carries a Python reference which needs to be decref'd. + /// + /// If the pointer is NULL, this function will fetch an error. unsafe fn assume_owned_or_err(self, py: Python<'_>) -> PyResult>; + + /// Same as `assume_owned_or_err`, but doesn't fetch an error on NULL. unsafe fn assume_owned_or_opt(self, py: Python<'_>) -> Option>; + + /// Same as `assume_owned_or_err`, but panics on NULL. unsafe fn assume_owned(self, py: Python<'_>) -> Bound<'_, PyAny>; + /// Same as `assume_owned_or_err`, but does not check for NULL. + unsafe fn assume_owned_unchecked(self, py: Python<'_>) -> Bound<'_, PyAny>; + /// Assumes this pointer is borrowed from a parent object. /// /// Warning: the lifetime `'a` is not bounded by the function arguments; the caller is @@ -44,6 +54,11 @@ impl FfiPtrExt for *mut ffi::PyObject { Bound::from_owned_ptr(py, self) } + #[inline] + unsafe fn assume_owned_unchecked(self, py: Python<'_>) -> Bound<'_, PyAny> { + Bound::from_owned_ptr_unchecked(py, self) + } + #[inline] unsafe fn assume_borrowed_or_err<'a>( self, diff --git a/src/instance.rs b/src/instance.rs index 1e15624929e..9a3af7ebecc 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -134,6 +134,18 @@ impl<'py> Bound<'py, PyAny> { Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) } + /// Constructs a new `Bound<'py, PyAny>` from a pointer without checking for null. + /// + /// # Safety + /// + /// - `ptr` must be a valid pointer to a Python object + pub(crate) unsafe fn from_owned_ptr_unchecked( + py: Python<'py>, + ptr: *mut ffi::PyObject, + ) -> Self { + Self(py, ManuallyDrop::new(Py::from_owned_ptr_unchecked(ptr))) + } + /// Constructs a new `Bound<'py, PyAny>` from a pointer by creating a new Python reference. /// Panics if `ptr` is null. /// @@ -1630,6 +1642,15 @@ impl Py { NonNull::new(ptr).map(|nonnull_ptr| Py(nonnull_ptr, PhantomData)) } + /// Constructs a new `Py` instance by taking ownership of the given FFI pointer. + /// + /// # Safety + /// + /// - `ptr` must be a non-null pointer to a Python object or type `T`. + pub(crate) unsafe fn from_owned_ptr_unchecked(ptr: *mut ffi::PyObject) -> Self { + Py(NonNull::new_unchecked(ptr), PhantomData) + } + /// Create a `Py` instance by creating a new reference from the given FFI pointer. /// /// # Safety diff --git a/src/pycell.rs b/src/pycell.rs index 438f7245a22..38f82eb27fd 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -370,7 +370,7 @@ where inner: unsafe { ManuallyDrop::new(self) .as_ptr() - .assume_owned(py) + .assume_owned_unchecked(py) .downcast_into_unchecked() }, } @@ -587,7 +587,7 @@ where inner: unsafe { ManuallyDrop::new(self) .as_ptr() - .assume_owned(py) + .assume_owned_unchecked(py) .downcast_into_unchecked() }, } diff --git a/src/types/dict.rs b/src/types/dict.rs index c4de7bae46a..61767f245cc 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -261,7 +261,11 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { } { std::os::raw::c_int::MIN..=-1 => Err(PyErr::fetch(py)), 0 => Ok(None), - 1..=std::os::raw::c_int::MAX => Ok(Some(unsafe { result.assume_owned(py) })), + 1..=std::os::raw::c_int::MAX => { + // Safety: PyDict_GetItemRef positive return value means the result is a valid + // owned reference + Ok(Some(unsafe { result.assume_owned_unchecked(py) })) + } } } diff --git a/src/types/weakref/anyref.rs b/src/types/weakref/anyref.rs index 06d91ea024e..a3632544684 100644 --- a/src/types/weakref/anyref.rs +++ b/src/types/weakref/anyref.rs @@ -395,7 +395,9 @@ impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakref> { match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } { std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref' weak reference instance should be valid (non-null and actually a weakref reference)"), 0 => PyNone::get(self.py()).to_owned().into_any(), - 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) }, + // Safety: positive return value from `PyWeakRef_GetRef` guarantees the return value is + // a valid strong reference. + 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned_unchecked(self.py()) }, } } } diff --git a/src/types/weakref/proxy.rs b/src/types/weakref/proxy.rs index 798b4b435c7..a16c7583882 100644 --- a/src/types/weakref/proxy.rs +++ b/src/types/weakref/proxy.rs @@ -187,7 +187,9 @@ impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakrefProxy> { match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } { std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref.ProxyType' (or `weakref.CallableProxyType`) instance should be valid (non-null and actually a weakref reference)"), 0 => PyNone::get(self.py()).to_owned().into_any(), - 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) }, + // Safety: positive return value from `PyWeakRef_GetRef` guarantees the return value is + // a valid strong reference. + 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned_unchecked(self.py()) }, } } } diff --git a/src/types/weakref/reference.rs b/src/types/weakref/reference.rs index cc8bc3d55f5..40890e0f887 100644 --- a/src/types/weakref/reference.rs +++ b/src/types/weakref/reference.rs @@ -196,7 +196,9 @@ impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakrefReference> { match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } { std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref.ReferenceType' instance should be valid (non-null and actually a weakref reference)"), 0 => PyNone::get(self.py()).to_owned().into_any(), - 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) }, + // Safety: positive return value from `PyWeakRef_GetRef` guarantees the return value is + // a valid strong reference. + 1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned_unchecked(self.py()) }, } } } From 395f9136dbcef1eba83984958d735bc58f6627eb Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 4 Oct 2024 12:50:58 +0100 Subject: [PATCH 2/2] Update src/instance.rs Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> --- src/instance.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/instance.rs b/src/instance.rs index 9a3af7ebecc..4d5608a9dc0 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -139,6 +139,7 @@ impl<'py> Bound<'py, PyAny> { /// # Safety /// /// - `ptr` must be a valid pointer to a Python object + /// - `ptr` must be a strong/owned reference pub(crate) unsafe fn from_owned_ptr_unchecked( py: Python<'py>, ptr: *mut ffi::PyObject,