safety: abort if dropping panic payloads panic

CHANGELOG.md

@@ -56,6 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `pyo3_build_config::add_extension_module_link_args()` now also emits linker arguments for `wasm32-unknown-emscripten`. [#2538](https://github.com/PyO3/pyo3/pull/2538)
 - Downcasting (`PyTryFrom`) behavior has changed for `PySequence` and `PyMapping`: classes are now required to inherit from (or register with) the corresponding Python standard library abstract base class. See the [migration guide](https://pyo3.rs/latest/migration.html) for information on fixing broken downcasts. [#2477](https://github.com/PyO3/pyo3/pull/2477)
 - Disable `PyFunction` on `Py_LIMITED_API` and PyPy. [#2542](https://github.com/PyO3/pyo3/pull/2542)
+- Panics during drop of panic payload caught by PyO3 will now abort. [#2544](https://github.com/PyO3/pyo3/pull/2544)
 
 ### Removed
 

pyo3-macros-backend/src/pymethod.rs

@@ -339,19 +339,20 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec<'_>) -> MethodAndSlotDef {
             arg: *mut ::std::os::raw::c_void,
         ) -> ::std::os::raw::c_int
         {
+            let trap = _pyo3::impl_::panic::PanicTrap::new("uncaught panic inside __traverse__ handler");
             let pool = _pyo3::GILPool::new();
             let py = pool.python();
-            _pyo3::callback::abort_on_traverse_panic(::std::panic::catch_unwind(move || {
-                let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
-
-                let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
-                let borrow = slf.try_borrow();
-                if let ::std::result::Result::Ok(borrow) = borrow {
-                    _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit))
-                } else {
-                    0
-                }
-            }))
+            let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
+
+            let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
+            let borrow = slf.try_borrow();
+            let retval = if let ::std::result::Result::Ok(borrow) = borrow {
+                _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit))
+            } else {
+                0
+            };
+            trap.disarm();
+            retval
         }
     };
     let slot_def = quote! {

src/callback.rs

@@ -5,6 +5,7 @@
 use crate::err::{PyErr, PyResult};
 use crate::exceptions::PyOverflowError;
 use crate::ffi::{self, Py_hash_t};
+use crate::impl_::panic::PanicTrap;
 use crate::panic::PanicException;
 use crate::{GILPool, IntoPyPointer};
 use crate::{IntoPy, PyObject, Python};
@@ -243,9 +244,15 @@ where
     F: for<'py> FnOnce(Python<'py>) -> PyResult<R> + UnwindSafe,
     R: PyCallbackOutput,
 {
+    let trap = PanicTrap::new("uncaught panic at ffi boundary");
     let pool = GILPool::new();
     let py = pool.python();
-    panic_result_into_callback_output(py, panic::catch_unwind(move || -> PyResult<_> { body(py) }))
+    let out = panic_result_into_callback_output(
+        py,
+        panic::catch_unwind(move || -> PyResult<_> { body(py) }),
+    );
+    trap.disarm();
+    out
 }
 
 /// Converts the output of std::panic::catch_unwind into a Python function output, either by raising a Python
@@ -259,28 +266,11 @@ pub fn panic_result_into_callback_output<R>(
 where
     R: PyCallbackOutput,
 {
-    let py_result = match panic_result {
-        Ok(py_result) => py_result,
-        Err(payload) => Err(PanicException::from_panic_payload(payload)),
+    let py_err = match panic_result {
+        Ok(Ok(value)) => return value,
+        Ok(Err(py_err)) => py_err,
+        Err(payload) => PanicException::from_panic_payload(payload),
     };
-
-    py_result.unwrap_or_else(|py_err| {
-        py_err.restore(py);
-        R::ERR_VALUE
-    })
-}
-
-/// Aborts if panic has occurred. Used inside `__traverse__` implementations, where panicking is not possible.
-#[doc(hidden)]
-#[inline]
-pub fn abort_on_traverse_panic(
-    panic_result: Result<c_int, Box<dyn Any + Send + 'static>>,
-) -> c_int {
-    match panic_result {
-        Ok(traverse_result) => traverse_result,
-        Err(_payload) => {
-            eprintln!("FATAL: panic inside __traverse__ handler; aborting.");
-            ::std::process::abort()
-        }
-    }
+    py_err.restore(py);
+    R::ERR_VALUE
 }

src/err/mod.rs

@@ -463,11 +463,14 @@ impl PyErr {
     /// This is the opposite of `PyErr::fetch()`.
     #[inline]
     pub fn restore(self, py: Python<'_>) {
-        let (ptype, pvalue, ptraceback) = self
-            .state
-            .into_inner()
-            .expect("Cannot restore a PyErr while normalizing it")
-            .into_ffi_tuple(py);
+        let state = match self.state.into_inner() {
+            Some(state) => state,
+            // Safety: restore takes `self` by value so nothing else is accessing this err
+            // and the invariant is that state is always defined except during make_normalized
+            None => unsafe { std::hint::unreachable_unchecked() },
+        };
+
+        let (ptype, pvalue, ptraceback) = state.into_ffi_tuple(py);
         unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) }
     }
 
@@ -527,6 +530,11 @@ impl PyErr {
         }
     }
 
+    pub(crate) fn write_unraisable(self, py: Python<'_>, context: PyObject) {
+        self.restore(py);
+        unsafe { ffi::PyErr_WriteUnraisable(context.as_ptr()) };
+    }
+
     #[inline]
     fn from_state(state: PyErrState) -> PyErr {
         PyErr {

src/impl_.rs

@@ -11,6 +11,7 @@ pub mod freelist;
 pub mod frompyobject;
 pub mod ghost;
 pub(crate) mod not_send;
+pub mod panic;
 #[doc(hidden)]
 pub mod pyclass;
 #[doc(hidden)]

src/impl_/panic.rs

@@ -0,0 +1,27 @@
+/// Type which will panic if dropped.
+///
+/// If this is dropped during a panic, this will cause an abort.
+///
+/// Use this to avoid letting unwinds cross through the FFI boundary, which is UB.
+pub struct PanicTrap {
+    msg: &'static str,
+}
+
+impl PanicTrap {
+    #[inline]
+    pub const fn new(msg: &'static str) -> Self {
+        Self { msg }
+    }
+
+    #[inline]
+    pub const fn disarm(self) {
+        std::mem::forget(self)
+    }
+}
+
+impl Drop for PanicTrap {
+    fn drop(&mut self) {
+        // Panic here will abort the process, assuming in an unwind.
+        panic!("{}", self.msg)
+    }
+}

src/panic.rs

@@ -16,7 +16,9 @@ Python interpreter to exit.
 );
 
 impl PanicException {
-    // Try to format the error in the same way panic does
+    /// Creates a new PanicException from a panic payload.
+    ///
+    /// Attempts to format the error in the same way panic does.
     #[cold]
     pub(crate) fn from_panic_payload(payload: Box<dyn Any + Send + 'static>) -> PyErr {
         if let Some(string) = payload.downcast_ref::<String>() {

src/types/function.rs

@@ -1,11 +1,13 @@
 use crate::derive_utils::PyFunctionArguments;
 use crate::exceptions::PyValueError;
-use crate::prelude::*;
+use crate::impl_::panic::PanicTrap;
+use crate::panic::PanicException;
 use crate::{
     ffi,
     impl_::pymethods::{self, PyMethodDef},
     types, AsPyPointer,
 };
+use crate::{prelude::*, GILPool};
 use std::os::raw::c_void;
 
 /// Represents a builtin Python function object.
@@ -40,20 +42,20 @@ where
     F: Fn(&types::PyTuple, Option<&types::PyDict>) -> R + Send + 'static,
     R: crate::callback::IntoPyCallbackOutput<*mut ffi::PyObject>,
 {
-    let result = std::panic::catch_unwind(|| {
+    let trap = PanicTrap::new("uncaught panic during drop_closure");
+    let pool = GILPool::new();
+    if let Err(payload) = std::panic::catch_unwind(|| {
         let boxed_fn: Box<F> = Box::from_raw(ffi::PyCapsule_GetPointer(
             capsule_ptr,
             CLOSURE_CAPSULE_NAME.as_ptr() as *const _,
         ) as *mut F);
         drop(boxed_fn)
-    });
-    if let Err(err) = result {
-        // This second layer of catch_unwind is useful as eprintln! can also panic.
-        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
-            eprintln!("--- PyO3 intercepted a panic when dropping a closure");
-            eprintln!("{:?}", err);
-        }));
-    }
+    }) {
+        let py = pool.python();
+        let err = PanicException::from_panic_payload(payload);
+        err.write_unraisable(py, "when dropping a closure".into_py(py));
+    };
+    trap.disarm();
 }
 
 impl PyCFunction {