From b083e0babf3d37100f57ffa0727bd5561be162b9 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 7 May 2020 14:46:47 +0100 Subject: [PATCH] Make allow_threads safe with panics --- CHANGELOG.md | 1 + src/python.rs | 40 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93ddef3bf45..0776c04a21e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * Garbage Collector causing random panics when traversing objects that were mutably borrowed. [#855](https://github.com/PyO3/pyo3/pull/855) * `&'static Py~` being allowed as arguments. [#869](https://github.com/PyO3/pyo3/pull/869) * `#[pyo3(get)]` for `Py`. [#880](https://github.com/PyO3/pyo3/pull/880) +* `allow_threads` will no longer cause segfaults in the event of panics inside the closure. [#912](https://github.com/PyO3/pyo3/pull/912) ### Removed * `PyMethodsProtocol` is now renamed to `PyMethodsImpl` and hidden. [#889](https://github.com/PyO3/pyo3/pull/889) diff --git a/src/python.rs b/src/python.rs index 14af58597db..fbb682c36a6 100644 --- a/src/python.rs +++ b/src/python.rs @@ -136,9 +136,16 @@ impl<'p> Python<'p> { // transferring the `Python` token into the closure. unsafe { let save = ffi::PyEval_SaveThread(); - let result = f(); + // Unwinding right here corrupts the Python interpreter state and leads to weird + // crashes such as stack overflows. We will catch the unwind and resume as soon as + // we've restored the GIL state. + // + // Because we will resume unwinding as soon as the GIL state is fixed, we can assert + // that the closure is unwind safe. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); ffi::PyEval_RestoreThread(save); - result + // Now that the GIL state has been safely reset, we can unwind if a panic was caught. + result.unwrap_or_else(|payload| std::panic::resume_unwind(payload)) } } @@ -469,4 +476,33 @@ mod test { assert!(py.is_subclass::().unwrap()); assert!(!py.is_subclass::().unwrap()); } + + #[test] + fn test_allow_threads_panics_safely() { + // If -Cpanic=abort is specified, we can't catch panic. + if option_env!("RUSTFLAGS") + .map(|s| s.contains("-Cpanic=abort")) + .unwrap_or(false) + { + return; + } + + let gil = Python::acquire_gil(); + let py = gil.python(); + + let result = std::panic::catch_unwind(|| unsafe { + let py = Python::assume_gil_acquired(); + py.allow_threads(|| { + panic!("There was a panic!"); + }); + }); + + // Check panic was caught + assert!(result.is_err()); + + // If allow_threads is implemented correctly, this thread still owns the GIL here + // so the following Python calls should not cause crashes. + let list = PyList::new(py, &[1, 2, 3, 4]); + assert_eq!(list.extract::>().unwrap(), vec![1, 2, 3, 4]); + } }