Panics should be caught at the FFI boundary and propagated into python some other way

[nagisa]

All Rust stack frames which have FFI stack frames directly under them should be guarded by a catch_unwind to ensure that it is impossible to accidentally panic back into Python’s stack frames. It is undefined behaviour to panic-unwind into stack frames of functions written in other languages, which makes any Rust-written python method that may panic for any reason – pyo3 is not exempt – unsound.

Alternatively, users of pyo3 should be instructed to wrap their code into catch_unwind and handle this scenario on their own. In that case the requirement to not panic without catch_unwind should be thoroughly documented.

---

For reference, I encountered this issue by experimenting with errors and doing something along the lines of:

#[pyclass]
struct Exception {};

// In a `PyResult` returning method
return Err(PyErr::new::<Exception, _>("hello"));

Where PyErr panicked because Exception is not a valid Exception type.

[davidhewitt]

I'm going to have a go at this in the next week or so - seems like a relatively self-contained change with big payoff for safety.

[programmerjake]

Related: rust-lang/rfcs#2797

[kngwyu]

seems like a relatively self-contained change

I don't think so, just because we have too many extern "C" fn wrap functions.
Maybe we need some system to ensure our wrap functions are called with catch_unwind.

[davidhewitt]

Agreed - what I meant is that only the wrap functions need to be affected, nothing else.

[davidhewitt]

Maybe we need some system to ensure our wrap functions are called with catch_unwind.

What I'm leaning towards is adding a new utility function pyo3::run_wrapped_rust() - and all wrap functions basically call this straight away.

I'm still iterating on the exact design of run_wrapped_rust. Expect to have a draft PR open in a few days' time.

[kngwyu]

What I'm leaning towards is adding a new utility function pyo3::run_wrapped_rust()

Sound nice 👍 , though I think it's also good to use catch_unwind directly to reduce the overhead of closure call.
And I recommend using pyo3_excpetion macro to declare the error type representing panic(e.g., PanickingError).

[kngwyu]

Hmm but I noticed we can't retrieve panic information from catch_unwind.
Its error type is Box<dyn Any + Send + 'static>, which does not have Display.
So... how about calling set_hook only for extensions, using the feature flag?
Then we can set Python exception in the hook, with full panic information.

[davidhewitt]

It is true that catch_unwind's result doesn't impl Display. However the unwind still occurs after set_hook, so that's not enough to stop unwinding through C.

We could maybe do something nice with set_hook for printing panic information, but catch_unwind will be necessary to stop the UB.

[kngwyu]

Ah, I understand, thanks.

[1tgr]

catch_unwind gives you a Box<dyn Any>, which you can downcast to String.

[davidhewitt]

catch_unwind gives you a Box, which you can downcast to String.

Yes, that's how I'm currently playing with formatting panic messages.