Skip to content

Commit

Permalink
Add catch_unwind! macro to prevent panics crossing ffi boundaries
Browse files Browse the repository at this point in the history
  • Loading branch information
davidhewitt committed May 5, 2020
1 parent c4f3653 commit a0eff02
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 8 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

### Changed

* Panics from Rust will now be caught and raised as Python errors. [#797](https://github.com/PyO3/pyo3/pull/797)
* `PyObject` and `Py<T>` reference counts are now decremented sooner after `drop()`. [#851](https://github.com/PyO3/pyo3/pull/851)
* When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.)
* When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.)
Expand Down
26 changes: 22 additions & 4 deletions src/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ where
/// It sets up the GILPool and converts the output into a Python object. It also restores
/// any python error returned as an Err variant from the body.
///
/// Finally, any panics inside the callback body will be caught and translated into PanicExceptions.
///
/// # Safety
/// This macro assumes the GIL is held. (It makes use of unsafe code, so usage of it is only
/// possible inside unsafe blocks.)
Expand Down Expand Up @@ -204,11 +206,27 @@ macro_rules! callback_body {
macro_rules! callback_body_without_convert {
($py:ident, $body:expr) => {{
let pool = $crate::GILPool::new();
let unwind_safe_py = std::panic::AssertUnwindSafe(pool.python());
let result = match std::panic::catch_unwind(move || -> $crate::PyResult<_> {
let $py = *unwind_safe_py;
$body
}) {
Ok(result) => result,
Err(e) => {
// Try to format the error in the same way panic does
if let Some(string) = e.downcast_ref::<String>() {
Err($crate::panic::PanicException::py_err((string.clone(),)))
} else if let Some(s) = e.downcast_ref::<&str>() {
Err($crate::panic::PanicException::py_err((s.to_string(),)))
} else {
Err($crate::panic::PanicException::py_err((
"panic from Rust code",
)))
}
}
};

let $py = pool.python();
let callback = move || -> $crate::PyResult<_> { $body };

callback().unwrap_or_else(|e| {
result.unwrap_or_else(|e| {
e.restore(pool.python());
$crate::callback::callback_error()
})
Expand Down
40 changes: 36 additions & 4 deletions src/err.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright (c) 2017-present PyO3 Project and Contributors

use crate::panic::PanicException;
use crate::type_object::PyTypeObject;
use crate::types::PyType;
use crate::{exceptions, ffi};
use crate::{
AsPyPointer, FromPy, IntoPy, IntoPyPointer, Py, PyAny, PyObject, Python, ToBorrowedObject,
ToPyObject,
AsPyPointer, FromPy, FromPyPointer, IntoPy, IntoPyPointer, ObjectProtocol, Py, PyAny, PyObject,
Python, ToBorrowedObject, ToPyObject,
};
use libc::c_int;
use std::ffi::CString;
Expand Down Expand Up @@ -168,13 +169,31 @@ impl PyErr {
///
/// The error is cleared from the Python interpreter.
/// If no error is set, returns a `SystemError`.
pub fn fetch(_: Python) -> PyErr {
///
/// If the error fetched is a `PanicException` (which would have originated from a panic in a
/// pyo3 callback) then this function will resume the panic.
pub fn fetch(py: Python) -> PyErr {
unsafe {
let mut ptype: *mut ffi::PyObject = std::ptr::null_mut();
let mut pvalue: *mut ffi::PyObject = std::ptr::null_mut();
let mut ptraceback: *mut ffi::PyObject = std::ptr::null_mut();
ffi::PyErr_Fetch(&mut ptype, &mut pvalue, &mut ptraceback);
PyErr::new_from_ffi_tuple(ptype, pvalue, ptraceback)

let err = PyErr::new_from_ffi_tuple(ptype, pvalue, ptraceback);

if ptype == PanicException::type_object().as_ptr() {
let msg: String = PyAny::from_borrowed_ptr_or_opt(py, pvalue)
.and_then(|obj| obj.extract().ok())
.unwrap_or_else(|| String::from("Unwrapped panic from Python code"));

eprintln!("--- PyO3 is resuming a panic after fetching a PanicException from Python. ---");
eprintln!("Python stack trace below:");
err.print(py);

std::panic::resume_unwind(Box::new(msg))
}

err
}
}

Expand Down Expand Up @@ -564,6 +583,7 @@ pub fn error_on_minusone(py: Python, result: c_int) -> PyResult<()> {
#[cfg(test)]
mod tests {
use crate::exceptions;
use crate::panic::PanicException;
use crate::{PyErr, Python};

#[test]
Expand All @@ -575,4 +595,16 @@ mod tests {
assert!(PyErr::occurred(py));
drop(PyErr::fetch(py));
}

#[test]
fn fetching_panic_exception_panics() {
let gil = Python::acquire_gil();
let py = gil.python();
let err: PyErr = PanicException::py_err("new panic");
err.restore(py);
assert!(PyErr::occurred(py));
let started_unwind =
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| PyErr::fetch(py))).is_err();
assert!(started_unwind);
}
}
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ mod internal_tricks;
pub mod marshal;
mod object;
mod objectprotocol;
#[macro_use]
pub mod panic;
pub mod prelude;
pub mod pycell;
pub mod pyclass;
Expand Down
12 changes: 12 additions & 0 deletions src/panic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use crate::exceptions::BaseException;

/// The exception raised when Rust code called from Python panics.
///
/// Like SystemExit, this exception is derived from BaseException so that
/// it will typically propagate all the way through the stack and cause the
/// Python interpreter to exit.
pub struct PanicException {
_private: (),
}

pyo3_exception!(PanicException, BaseException);

0 comments on commit a0eff02

Please sign in to comment.