run_closure and drop_closure unsoundly drop payload on panic

[LegionMammal978]

Bug Description

These two functions use catch_unwind to catch panics and transfer them as Python exceptions (for run_closure) or just report them (for drop_closure). However, both functions drop the payload returned by catch_unwind, which is an operation that can panic. Formally, unwinding from extern "C" functions is undefined behavior, but in practice, the program will effectively longjmp out of any C code as it unwinds. While this appears not to cause obvious memory unsafety in CPython, it can cause reference leaks and resource leaks. In particular, unwinding from run_closure prevents the recursion depth from being decremented, and unwinding from drop_closure prevents PyO3 from ever releasing the GIL and also prevents the trashcan mechanism from freeing any objects.

Steps to Reproduce

An example of a recursion depth leak from run_closure:

/*
[dependencies]
pyo3 = { path = "pyo3", default-features = false }
*/

use pyo3::{types::PyCFunction, Python};
use std::panic::{self, AssertUnwindSafe};

struct Bomb;

impl Drop for Bomb {
    fn drop(&mut self) {
        panic!();
    }
}

fn main() {
    pyo3::prepare_freethreaded_python();
    Python::with_gil(|py| {
        let f = PyCFunction::new_closure(|_, _| panic::panic_any(Bomb), py).unwrap();
        panic::set_hook(Box::new(|_| {}));
        for _ in 0..996 {
            panic::catch_unwind(AssertUnwindSafe(|| f.call0().unwrap())).unwrap_err();
        }
        let _ = panic::take_hook();
        py.eval("print('Hello, world!')", None, None).unwrap();
    });
}

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: PyErr { type: <class 'RecursionError'>, value: RecursionError('maximum recursion depth exceeded while calling a Python object'), traceback: Some(<traceback object at 0x7f389f58d940>) }', src/main.rs:38:55
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

An example of a GIL leak from drop_closure:

/*
[dependencies]
pyo3 = { path = "pyo3", default-features = false }
pyo3-ffi = { path = "pyo3/pyo3-ffi" }
*/

use pyo3::{types::PyCFunction, Python};
use std::panic;

struct Bomb;

impl Drop for Bomb {
    fn drop(&mut self) {
        panic!();
    }
}

struct Fuse;

impl Drop for Fuse {
    fn drop(&mut self) {
        panic::panic_any(Bomb);
    }
}

fn main() {
    pyo3::prepare_freethreaded_python();
    panic::set_hook(Box::new(|_| {}));
    let fuse = Fuse;
    panic::catch_unwind(|| {
        Python::with_gil(|py| {
            PyCFunction::new_closure(
                move |_, _| {
                    let _ = &fuse;
                },
                py,
            )
            .unwrap();
        })
    })
    .unwrap_err();
    let _ = panic::take_hook();
    let state = unsafe { pyo3_ffi::PyGILState_Check() };
    println!("GIL state: {state}");
}

--- PyO3 intercepted a panic when dropping a closure
Any { .. }
GIL state: 1

Backtrace

No response

Your operating system and version

Ubuntu 22.04 LTS

Your Python version (python --version)

Python 3.10.4

Your Rust version (rustc --version)

rustc 1.62.0 (a8314ef7d 2022-06-27)

Your PyO3 version

commit a95485c

How did you install python? Did you use a virtualenv?

apt

Additional Info

The error message in drop_closure can only ever print Any { .. }. To print the actual payload, it must be explicitly converted to a string with .downcast_ref::<&str>() and .downcast_ref::<String>().

[davidhewitt]

Thanks for reporting! Agree this is a bug - I'll put a fix into the upcoming 0.17 release.

[davidhewitt]

@LegionMammal978 I'll be looking at this very soon. I see you've been very active regarding this topic on the internals forum. I was wondering if you could confirm what the recommended fix is? I was thinking to add a second catch_unwind around the panic payload drop and then just leak any unwind payload caught there?

[mejrs]

I asked (someone else) a while ago and they recommended that approach. Personally I think this is very misbehaved and kind of a ridiculous corner case so my preference would be to just abort instead.

[LegionMammal978]

Both are acceptable. In function::drop_closure(), you can either write mem::forget(err), mem::forget(catch_unwind(|| drop(err))), or catch_unwind(|| drop(err)).unwrap_or_else(|_| process::abort()) (maybe with an extra AssertUnwindSafe). You can do the same in PanicException::from_panic_payload(). Also, the eprintln!() in function::drop_closure() requires the full downcast_ref() logic to actually print the message.

[davidhewitt]

I was thinking about this further and I think I'll go for the abort option - panic in panic is an abort, so it seems reasonable that panic on drop of panic payload is also an abort. Both are corner cases we hope never occur in practice!