From f6612c6dcf3295d9b6d342cf33a5d086af0b3e36 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 5 Oct 2025 21:54:19 +0100 Subject: [PATCH 1/6] use `PyErr::write_unraisable` when panicking --- src/err/mod.rs | 8 --- src/instance.rs | 147 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 118 insertions(+), 37 deletions(-) diff --git a/src/err/mod.rs b/src/err/mod.rs index 6575263aeeb..608eaa0c1d5 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -878,14 +878,6 @@ impl std::fmt::Display for TypeNameOrValue<'_> { } } -#[track_caller] -pub fn panic_after_error(_py: Python<'_>) -> ! { - unsafe { - ffi::PyErr_Print(); - } - panic!("Python API call failed"); -} - /// Returns Ok if the error code is not -1. #[inline] pub(crate) fn error_on_minusone(py: Python<'_>, result: T) -> PyResult<()> { diff --git a/src/instance.rs b/src/instance.rs index f25b44fe0df..a838322e525 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -327,7 +327,7 @@ impl<'py> Bound<'py, PyAny> { /// /// # Safety /// - /// - `ptr` must be a valid pointer to a Python object + /// - `ptr` must be a valid pointer to a Python object (or null, which will cause a panic) /// - `ptr` must be an owned Python reference, as the `Bound<'py, PyAny>` will assume ownership #[inline] #[track_caller] @@ -1058,18 +1058,16 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// /// # Safety /// - /// - `ptr` must be a valid pointer to a Python object + /// - `ptr` must be a valid pointer to a Python object (or null, which will cause a panic) /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by /// the caller and it is the caller's responsibility to ensure that the reference this is /// derived from is valid for the lifetime `'a`. #[inline] #[track_caller] pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { - Self( - NonNull::new(ptr).unwrap_or_else(|| crate::err::panic_after_error(py)), - PhantomData, - py, - ) + let non_null = NonNull::new(ptr).unwrap_or_else(|| panic_on_null(py)); + // SAFETY: caller upheld the safety contract, ptr is now known to be non-null + unsafe { Self::from_non_null(py, non_null) } } /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. @@ -1085,7 +1083,10 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// derived from is valid for the lifetime `'a`. #[inline] pub unsafe fn from_ptr_or_opt(py: Python<'py>, ptr: *mut ffi::PyObject) -> Option { - NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData, py)) + NonNull::new(ptr).map(|ptr| { + // SAFETY: caller upheld the safety contract, ptr is now known to be non-null + unsafe { Self::from_non_null(py, ptr) } + }) } /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch` @@ -1104,6 +1105,7 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { pub unsafe fn from_ptr_or_err(py: Python<'py>, ptr: *mut ffi::PyObject) -> PyResult { NonNull::new(ptr).map_or_else( || Err(PyErr::fetch(py)), + // SAFETY: caller upheld the safety contract, ptr is now known to be non-null |ptr| Ok(Self(ptr, PhantomData, py)), ) } @@ -1122,6 +1124,7 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// completely defined by the caller and it is the caller's responsibility /// to ensure that the reference this is derived from is valid for the /// lifetime `'a`. + #[inline(always)] pub(crate) unsafe fn from_non_null(py: Python<'py>, ptr: NonNull) -> Self { Self(ptr, PhantomData, py) } @@ -1902,20 +1905,27 @@ impl Py { /// Create a `Py` instance by taking ownership of the given FFI pointer. /// /// # Safety - /// `ptr` must be a pointer to a Python object of type T. /// - /// Callers must own the object referred to by `ptr`, as this function - /// implicitly takes ownership of that object. + /// - `ptr` must be a valid pointer to a Python object (or null, which will cause a panic) + /// - `ptr` must be an owned Python reference, as the `Py` will assume ownership /// /// # Panics + /// /// Panics if `ptr` is null. #[inline] #[track_caller] pub unsafe fn from_owned_ptr(py: Python<'_>, ptr: *mut ffi::PyObject) -> Py { - match NonNull::new(ptr) { - Some(nonnull_ptr) => Py(nonnull_ptr, PhantomData), - None => crate::err::panic_after_error(py), + // TODO: possibly make this constructor not generic + // https://github.com/PyO3/pyo3/pull/5495 + fn inner(py: Python<'_>, ptr: *mut ffi::PyObject) -> Py { + let non_null = NonNull::new(ptr).unwrap_or_else(|| panic_on_null(py)); + // SAFETY: caller provided a valid object, now known to be non-null + unsafe { Py::from_non_null(non_null) } } + + let object = inner(py, ptr).into_bound(py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() } /// Create a `Py` instance by taking ownership of the given FFI pointer. @@ -1923,16 +1933,29 @@ impl Py { /// If `ptr` is null then the current Python exception is fetched as a [`PyErr`]. /// /// # Safety - /// If non-null, `ptr` must be a pointer to a Python object of type T. + /// + /// - `ptr` must be a valid pointer to a Python object, or null + /// - a non-null `ptr` must be an owned Python reference, as the `Py` will assume ownership #[inline] pub unsafe fn from_owned_ptr_or_err( py: Python<'_>, ptr: *mut ffi::PyObject, ) -> PyResult> { - match NonNull::new(ptr) { - Some(nonnull_ptr) => Ok(Py(nonnull_ptr, PhantomData)), - None => Err(PyErr::fetch(py)), + // TODO: possibly make this constructor not generic + // https://github.com/PyO3/pyo3/pull/5495 + fn inner(py: Python<'_>, ptr: *mut ffi::PyObject) -> PyResult> { + NonNull::new(ptr).map_or_else( + || Err(PyErr::fetch(py)), + // SAFETY: caller provided a valid object, now known to be non-null + |nonnull_ptr| Ok(unsafe { Py::from_non_null(nonnull_ptr) }), + ) } + + inner(py, ptr).map(|object| { + let object = object.into_bound(py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() + }) } /// Create a `Py` instance by taking ownership of the given FFI pointer. @@ -1940,10 +1963,24 @@ impl Py { /// If `ptr` is null then `None` is returned. /// /// # Safety - /// If non-null, `ptr` must be a pointer to a Python object of type T. + /// + /// - `ptr` must be a valid pointer to a Python object, or null + /// - a non-null `ptr` must be an owned Python reference, as the `Py` will assume ownership #[inline] pub unsafe fn from_owned_ptr_or_opt(_py: Python<'_>, ptr: *mut ffi::PyObject) -> Option { - NonNull::new(ptr).map(|nonnull_ptr| Py(nonnull_ptr, PhantomData)) + // TODO: possibly make this constructor not generic + fn inner(_py: Python<'_>, ptr: *mut ffi::PyObject) -> Option> { + NonNull::new(ptr).map(|nonnull_ptr| { + // SAFETY: caller provided a valid object, now known to be non-null + unsafe { Py::from_non_null(nonnull_ptr) } + }) + } + + inner(_py, ptr).map(|object| { + let object = object.into_bound(_py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() + }) } /// Constructs a new `Py` instance by taking ownership of the given FFI pointer. @@ -1952,7 +1989,8 @@ impl Py { /// /// - `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(unsafe { NonNull::new_unchecked(ptr) }, PhantomData) + // SAFETY: caller upheld the safety contract + unsafe { Py::from_non_null(NonNull::new_unchecked(ptr)) } } /// Create a `Py` instance by creating a new reference from the given FFI pointer. @@ -1961,14 +1999,22 @@ impl Py { /// `ptr` must be a pointer to a Python object of type T. /// /// # Panics + /// /// Panics if `ptr` is null. #[inline] #[track_caller] pub unsafe fn from_borrowed_ptr(py: Python<'_>, ptr: *mut ffi::PyObject) -> Py { - match unsafe { Self::from_borrowed_ptr_or_opt(py, ptr) } { - Some(slf) => slf, - None => crate::err::panic_after_error(py), + // TODO: possibly make this constructor not generic + // https://github.com/PyO3/pyo3/pull/5495 + fn inner(py: Python<'_>, ptr: *mut ffi::PyObject) -> Py { + let non_null = NonNull::new(ptr).unwrap_or_else(|| panic_on_null(py)); + // SAFETY: caller provided a valid borrowed object, now known to be non-null + unsafe { Py::from_borrowed_non_null(py, non_null) } } + + let object = inner(py, ptr).into_bound(py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() } /// Create a `Py` instance by creating a new reference from the given FFI pointer. @@ -1982,7 +2028,21 @@ impl Py { py: Python<'_>, ptr: *mut ffi::PyObject, ) -> PyResult { - unsafe { Self::from_borrowed_ptr_or_opt(py, ptr).ok_or_else(|| PyErr::fetch(py)) } + // TODO: possibly make this constructor not generic + // https://github.com/PyO3/pyo3/pull/5495 + fn inner(py: Python<'_>, ptr: *mut ffi::PyObject) -> PyResult> { + NonNull::new(ptr).map_or_else( + || Err(PyErr::fetch(py)), + // SAFETY: caller provided a valid borrowed object, now known to be non-null + |nonnull_ptr| Ok(unsafe { Py::from_borrowed_non_null(py, nonnull_ptr) }), + ) + } + + inner(py, ptr).map(|object| { + let object = object.into_bound(py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() + }) } /// Create a `Py` instance by creating a new reference from the given FFI pointer. @@ -1996,21 +2056,41 @@ impl Py { _py: Python<'_>, ptr: *mut ffi::PyObject, ) -> Option { - unsafe { + // TODO: possibly make this constructor not generic + fn inner(_py: Python<'_>, ptr: *mut ffi::PyObject) -> Option> { NonNull::new(ptr).map(|nonnull_ptr| { - ffi::Py_INCREF(ptr); - Py(nonnull_ptr, PhantomData) + // SAFETY: caller provided a valid borrowed object, now known to be non-null + unsafe { Py::from_borrowed_non_null(_py, nonnull_ptr) } }) } + + inner(_py, ptr).map(|object| { + let object = object.into_bound(_py); + // SAFETY: caller provided an object of type T. + unsafe { object.cast_into_unchecked() }.unbind() + }) } /// For internal conversions. /// /// # Safety - /// `ptr` must point to a Python object of type T. + /// `ptr` must be an owning reference pointing to a Python object of type T. + #[inline] unsafe fn from_non_null(ptr: NonNull) -> Self { Self(ptr, PhantomData) } + + /// For internal conversions. + /// + /// # Safety + /// `ptr` must be a borrowed reference pointing to a Python object of type T. + #[inline] + unsafe fn from_borrowed_non_null(_py: Python<'_>, ptr: NonNull) -> Self { + // SAFETY: caller provided a valid object, known to be non-null, which needs to have + // its reference count increased, and the thread is attached to the Python interpreter. + unsafe { ffi::Py_INCREF(ptr.as_ptr()) }; + Self(ptr, PhantomData) + } } impl AsRef> for Py { @@ -2338,6 +2418,15 @@ impl Py { } } +#[track_caller] +#[cold] +fn panic_on_null(py: Python<'_>) -> ! { + if let Some(err) = PyErr::take(py) { + err.write_unraisable(py, None); + } + panic!("PyObject pointer is null"); +} + #[cfg(test)] mod tests { use super::{Bound, IntoPyObject, Py}; From 207f7d4cdd8bb14107a5099e850860561c5d1bab Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 1 Nov 2025 19:48:11 +0000 Subject: [PATCH 2/6] add a test --- src/instance.rs | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/instance.rs b/src/instance.rs index 181c580c6e5..42b0b70d9b4 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2445,7 +2445,9 @@ fn panic_on_null(py: Python<'_>) -> ! { #[cfg(test)] mod tests { use super::{Bound, IntoPyObject, Py}; + use crate::exceptions::PyValueError; use crate::test_utils::generate_unique_module_name; + use crate::test_utils::UnraisableCapture; use crate::types::{dict::IntoPyDict, PyAnyMethods, PyCapsule, PyDict, PyString}; use crate::{ffi, Borrowed, IntoPyObjectExt, PyAny, PyResult, Python}; use std::ffi::CStr; @@ -2802,6 +2804,55 @@ a = A() }); } + #[test] + fn test_constructors_panic_on_null() { + Python::attach(|py| { + #[expect(deprecated, reason = "Py constructors")] + for constructor in [ + (|py, ptr| unsafe { + Py::::from_owned_ptr(py, ptr); + }) as fn(Python<'_>, *mut ffi::PyObject), + (|py, ptr| unsafe { + Py::::from_borrowed_ptr(py, ptr); + }) as fn(Python<'_>, *mut ffi::PyObject), + (|py, ptr| unsafe { + Bound::from_owned_ptr(py, ptr); + }) as fn(Python<'_>, *mut ffi::PyObject), + (|py, ptr| unsafe { + Bound::from_borrowed_ptr(py, ptr); + }) as fn(Python<'_>, *mut ffi::PyObject), + (|py, ptr| unsafe { + Borrowed::from_ptr(py, ptr); + }) as fn(Python<'_>, *mut ffi::PyObject), + ] { + UnraisableCapture::enter(py, |capture| { + // panic without exception set, no unraisable hook called + let result = std::panic::catch_unwind(|| { + constructor(py, std::ptr::null_mut()); + }); + assert_eq!( + result.unwrap_err().downcast_ref::<&str>(), + Some(&"PyObject pointer is null") + ); + assert!(capture.take_capture().is_none()); + + // set an exception, panic, unraisable hook called + PyValueError::new_err("error").restore(py); + let result = std::panic::catch_unwind(|| { + constructor(py, std::ptr::null_mut()); + }); + assert_eq!( + result.unwrap_err().downcast_ref::<&str>(), + Some(&"PyObject pointer is null") + ); + assert!(capture.take_capture().is_some_and(|(err, obj)| { + err.is_instance_of::(py) && obj.is_none() + })); + }); + } + }); + } + #[cfg(feature = "macros")] mod using_macros { use super::*; From 87c3ede6a67bce28e345f01c6bb3081779ada101 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 1 Nov 2025 20:28:01 +0000 Subject: [PATCH 3/6] fixup cfg for 3.7 --- src/instance.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/instance.rs b/src/instance.rs index 42b0b70d9b4..82bc7d2d3a5 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2445,8 +2445,10 @@ fn panic_on_null(py: Python<'_>) -> ! { #[cfg(test)] mod tests { use super::{Bound, IntoPyObject, Py}; + #[cfg(all(feature = "macros", Py_3_8))] use crate::exceptions::PyValueError; use crate::test_utils::generate_unique_module_name; + #[cfg(all(feature = "macros", Py_3_8))] use crate::test_utils::UnraisableCapture; use crate::types::{dict::IntoPyDict, PyAnyMethods, PyCapsule, PyDict, PyString}; use crate::{ffi, Borrowed, IntoPyObjectExt, PyAny, PyResult, Python}; @@ -2804,6 +2806,7 @@ a = A() }); } + #[cfg(all(feature = "macros", Py_3_8))] #[test] fn test_constructors_panic_on_null() { Python::attach(|py| { From 8b24bc47305e16ad376243127de33872163c3c94 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 1 Nov 2025 20:29:24 +0000 Subject: [PATCH 4/6] newsfragment --- newsfragments/5496.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/5496.changed.md diff --git a/newsfragments/5496.changed.md b/newsfragments/5496.changed.md new file mode 100644 index 00000000000..9bd9982d649 --- /dev/null +++ b/newsfragments/5496.changed.md @@ -0,0 +1 @@ +Call `sys.unraisablehook` instead of `PyErr_Print` if panicking on null FFI pointer in `Bound`, `Borrowed` and `Py` constructors. From d3025b4be0c61bbf6e3513c4e68392612a03cdd8 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 3 Nov 2025 08:43:48 +0000 Subject: [PATCH 5/6] clippy --- src/instance.rs | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/src/instance.rs b/src/instance.rs index 82bc7d2d3a5..bb6d3f0771b 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2810,28 +2810,33 @@ a = A() #[test] fn test_constructors_panic_on_null() { Python::attach(|py| { + const NULL: *mut ffi::PyObject = std::ptr::null_mut(); + #[expect(deprecated, reason = "Py constructors")] - for constructor in [ - (|py, ptr| unsafe { - Py::::from_owned_ptr(py, ptr); - }) as fn(Python<'_>, *mut ffi::PyObject), - (|py, ptr| unsafe { - Py::::from_borrowed_ptr(py, ptr); - }) as fn(Python<'_>, *mut ffi::PyObject), - (|py, ptr| unsafe { - Bound::from_owned_ptr(py, ptr); - }) as fn(Python<'_>, *mut ffi::PyObject), - (|py, ptr| unsafe { - Bound::from_borrowed_ptr(py, ptr); - }) as fn(Python<'_>, *mut ffi::PyObject), - (|py, ptr| unsafe { - Borrowed::from_ptr(py, ptr); - }) as fn(Python<'_>, *mut ffi::PyObject), - ] { + // SAFETY: calling all constructors with null pointer to test panic behavior + for constructor in unsafe { + [ + (|py| { + Py::::from_owned_ptr(py, NULL); + }) as fn(Python<'_>), + (|py| { + Py::::from_borrowed_ptr(py, NULL); + }) as fn(Python<'_>), + (|py| { + Bound::from_owned_ptr(py, NULL); + }) as fn(Python<'_>), + (|py| { + Bound::from_borrowed_ptr(py, NULL); + }) as fn(Python<'_>), + (|py| { + Borrowed::from_ptr(py, NULL); + }) as fn(Python<'_>), + ] + } { UnraisableCapture::enter(py, |capture| { // panic without exception set, no unraisable hook called let result = std::panic::catch_unwind(|| { - constructor(py, std::ptr::null_mut()); + constructor(py); }); assert_eq!( result.unwrap_err().downcast_ref::<&str>(), @@ -2842,7 +2847,7 @@ a = A() // set an exception, panic, unraisable hook called PyValueError::new_err("error").restore(py); let result = std::panic::catch_unwind(|| { - constructor(py, std::ptr::null_mut()); + constructor(py); }); assert_eq!( result.unwrap_err().downcast_ref::<&str>(), From 0a74e8adefef0f437ed1545aedafe39f4460dc2b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 3 Nov 2025 08:45:29 +0000 Subject: [PATCH 6/6] add panic sections --- src/instance.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/instance.rs b/src/instance.rs index bb6d3f0771b..594de8e6f45 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -338,6 +338,10 @@ impl<'py> Bound<'py, PyAny> { /// /// - `ptr` must be a valid pointer to a Python object (or null, which will cause a panic) /// - `ptr` must be an owned Python reference, as the `Bound<'py, PyAny>` will assume ownership + /// + /// # Panics + /// + /// Panics if `ptr` is null. #[inline] #[track_caller] pub unsafe fn from_owned_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { @@ -401,6 +405,10 @@ impl<'py> Bound<'py, PyAny> { /// # Safety /// /// - `ptr` must be a valid pointer to a Python object + /// + /// # Panics + /// + /// Panics if `ptr` is null #[inline] #[track_caller] pub unsafe fn from_borrowed_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { @@ -1091,6 +1099,10 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by /// the caller and it is the caller's responsibility to ensure that the reference this is /// derived from is valid for the lifetime `'a`. + /// + /// # Panics + /// + /// Panics if `ptr` is null #[inline] #[track_caller] pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self {