diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ee61febb42..9b7bb34b9db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fix creating `#[classattr]` by functions with the name of a known magic method. [#1969](https://github.com/PyO3/pyo3/pull/1969) +- Fix use of `catch_unwind` in `allow_threads` which can cause fatal crashes. [#1989](https://github.com/PyO3/pyo3/pull/1989) - Fix build failure on PyPy when abi3 features are activated. [#1991](https://github.com/PyO3/pyo3/pull/1991) - Fix mingw platform detection. [#1993](https://github.com/PyO3/pyo3/pull/1993) diff --git a/src/python.rs b/src/python.rs index 4f2fdceb5d3..911bf6bda48 100644 --- a/src/python.rs +++ b/src/python.rs @@ -326,7 +326,7 @@ impl<'py> Python<'py> { /// py.allow_threads(move || { /// // An example of an "expensive" Rust calculation /// let sum = numbers.iter().sum(); - /// + /// /// Ok(sum) /// }) /// } @@ -367,26 +367,30 @@ impl<'py> Python<'py> { F: Send + FnOnce() -> T, T: Send, { + // Use a guard pattern to handle reacquiring the GIL, so that the GIL will be reacquired + // even if `f` panics. + + struct RestoreGuard { + count: usize, + tstate: *mut ffi::PyThreadState, + } + + impl Drop for RestoreGuard { + fn drop(&mut self) { + gil::GIL_COUNT.with(|c| c.set(self.count)); + unsafe { + ffi::PyEval_RestoreThread(self.tstate); + } + } + } + // The `Send` bound on the closure prevents the user from // transferring the `Python` token into the closure. let count = gil::GIL_COUNT.with(|c| c.replace(0)); let tstate = unsafe { ffi::PyEval_SaveThread() }; - // 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)); - - // Restore GIL state - gil::GIL_COUNT.with(|c| c.set(count)); - unsafe { - ffi::PyEval_RestoreThread(tstate); - } - // 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)) + let _guard = RestoreGuard { count, tstate }; + f() } /// Evaluates a Python expression in the given context and returns the result. @@ -840,6 +844,29 @@ mod tests { }); } + #[test] + fn test_allow_threads_releases_and_acquires_gil() { + Python::with_gil(|py| { + let b = std::sync::Arc::new(std::sync::Barrier::new(2)); + + let b2 = b.clone(); + std::thread::spawn(move || Python::with_gil(|_| b2.wait())); + + py.allow_threads(|| { + // If allow_threads does not release the GIL, this will deadlock because + // the thread spawned above will never be able to acquire the GIL. + b.wait(); + }); + + unsafe { + // If the GIL is not reacquired at the end of allow_threads, this call + // will crash the Python interpreter. + let tstate = ffi::PyEval_SaveThread(); + ffi::PyEval_RestoreThread(tstate); + } + }); + } + #[test] fn test_allow_threads_panics_safely() { Python::with_gil(|py| {