safety: abort if dropping panic payloads panic

CHANGELOG.md

@@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Only allow each `#[pymodule]` to be initialized once. [#2523](https://github.com/PyO3/pyo3/pull/2523)
 - `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)
+- Panics during drop of panic payload caught by PyO3 will now abort. [#2544](https://github.com/PyO3/pyo3/pull/2544)
 
 ### Removed
 

src/callback.rs

@@ -5,7 +5,7 @@
 use crate::err::{PyErr, PyResult};
 use crate::exceptions::PyOverflowError;
 use crate::ffi::{self, Py_hash_t};
-use crate::panic::PanicException;
+use crate::panic::{abort_with_msg, PanicException};
 use crate::{GILPool, IntoPyPointer};
 use crate::{IntoPy, PyObject, Python};
 use std::any::Any;
@@ -259,15 +259,14 @@ 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
-    })
+    py_err.restore(py);
+    R::ERR_VALUE
 }
 
 /// Aborts if panic has occurred. Used inside `__traverse__` implementations, where panicking is not possible.
@@ -278,9 +277,6 @@ pub fn abort_on_traverse_panic(
 ) -> c_int {
     match panic_result {
         Ok(traverse_result) => traverse_result,
-        Err(_payload) => {
-            eprintln!("FATAL: panic inside __traverse__ handler; aborting.");
-            ::std::process::abort()
-        }
+        Err(_payload) => abort_with_msg("panic inside __traverse__ handler"),
     }
 }

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/panic.rs

@@ -2,6 +2,7 @@
 use crate::exceptions::PyBaseException;
 use crate::PyErr;
 use std::any::Any;
+use std::io::Write;
 
 pyo3_exception!(
     "
@@ -16,15 +17,46 @@ 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.
+    ///
+    /// If dropping `payload` panics, will fatally abort.
     #[cold]
     pub(crate) fn from_panic_payload(payload: Box<dyn Any + Send + 'static>) -> PyErr {
-        if let Some(string) = payload.downcast_ref::<String>() {
-            Self::new_err((string.clone(),))
-        } else if let Some(s) = payload.downcast_ref::<&str>() {
-            Self::new_err((s.to_string(),))
+        let err = if let Some(msg) = panic_payload_to_string(payload) {
+            Self::new_err((msg,))
         } else {
             Self::new_err(("panic from Rust code",))
-        }
+        };
+        err
     }
 }
+
+/// Formats a panic payload into a string, and drops the panic payload.
+///
+/// If dropping the panic payload panics, aborts the process.
+pub(crate) fn panic_payload_to_string(payload: Box<dyn Any + Send + 'static>) -> Option<String> {
+    let msg = if let Some(string) = payload.downcast_ref::<String>() {
+        Some(string.clone())
+    } else if let Some(s) = payload.downcast_ref::<&str>() {
+        Some(s.to_string())
+    } else {
+        None
+    };
+    abort_on_unwind(move || drop(payload), "panic while dropping panic payload");
+    msg
+}
+
+/// Executes `f()`. If it panics, attempts to write `msg` to stderr and then aborts.
+pub(crate) fn abort_on_unwind<F: FnOnce()>(f: F, msg: &str) {
+    // Always fine to use AssertUnwindSafe here because a panic will abort.
+    std::panic::catch_unwind(std::panic::AssertUnwindSafe(f))
+        .unwrap_or_else(|_| abort_with_msg(msg));
+}
+
+/// Attempts to write `msg` to stderr, then aborts.
+pub(crate) fn abort_with_msg(msg: &str) -> ! {
+    let _ = writeln!(std::io::stderr(), "FATAL: {}, aborting", msg);
+    std::process::abort()
+}

src/types/function.rs

@@ -1,11 +1,12 @@
 use crate::derive_utils::PyFunctionArguments;
 use crate::exceptions::PyValueError;
-use crate::prelude::*;
+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 +41,19 @@ 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 pool = GILPool::new();
+    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)
+    })
+    .unwrap_or_else(|payload| {
+        let py = pool.python();
+        let err = PanicException::from_panic_payload(payload);
+        err.write_unraisable(py, "when dropping a closure".into_py(py));
     });
-    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);
-        }));
-    }
 }
 
 impl PyCFunction {