diff --git a/CHANGELOG.md b/CHANGELOG.md index d8b28942c07..06e89b45ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,9 +55,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Only allow each `#[pymodule]` to be initialized once. [#2523](https://github.com/PyO3/pyo3/pull/2523) - `pyo3_build_config::add_extension_module_link_args()` now also emits linker arguments for `wasm32-unknown-emscripten`. [#2538](https://github.com/PyO3/pyo3/pull/2538) - Downcasting (`PyTryFrom`) behavior has changed for `PySequence` and `PyMapping`: classes are now required to inherit from (or register with) the corresponding Python standard library abstract base class. See the [migration guide](https://pyo3.rs/latest/migration.html) for information on fixing broken downcasts. [#2477](https://github.com/PyO3/pyo3/pull/2477) -- Deprecated `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). This change was made for several reasons: - - GIL drop order tracking has turned out to be [error prone](https://github.com/PyO3/pyo3/issues/1683). With a scoped API like `Python::with_gil`, these are always dropped in the correct order. - - It promotes passing and keeping the GILGuard around, which is almost always not what you actually want. +- Disable `PyFunction` on `Py_LIMITED_API` and PyPy. [#2542](https://github.com/PyO3/pyo3/pull/2542) +- Panics during drop of panic payload caught by PyO3 will now abort. [#2544](https://github.com/PyO3/pyo3/pull/2544) +- Deprecate `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). ### Removed diff --git a/benches/bench_gil.rs b/benches/bench_gil.rs index 1ef515d98ef..e7290644cd0 100644 --- a/benches/bench_gil.rs +++ b/benches/bench_gil.rs @@ -12,9 +12,7 @@ fn bench_clean_gilpool_new(b: &mut Bencher<'_>) { fn bench_clean_acquire_gil(b: &mut Bencher<'_>) { // Acquiring first GIL will also create a "clean" GILPool, so this measures the Python overhead. - b.iter(|| { - let _ = Python::with_gil(|_| {}); - }); + b.iter(|| Python::with_gil(|_| {})); } fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { @@ -24,9 +22,7 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { // Clone and drop an object so that the GILPool has work to do. let _ = obj.clone(); }, - |_| { - let _ = Python::with_gil(|_| {}); - }, + |_| Python::with_gil(|_| {}), BatchSize::NumBatches(1), ); } diff --git a/pyo3-ffi/src/funcobject.rs b/pyo3-ffi/src/cpython/funcobject.rs similarity index 88% rename from pyo3-ffi/src/funcobject.rs rename to pyo3-ffi/src/cpython/funcobject.rs index 6366e363ccc..1d36c85ecbe 100644 --- a/pyo3-ffi/src/funcobject.rs +++ b/pyo3-ffi/src/cpython/funcobject.rs @@ -1,8 +1,8 @@ use std::os::raw::c_int; -use crate::object::{PyObject, PyTypeObject, Py_TYPE}; +use crate::PyObject; -#[cfg(all(not(PyPy), not(Py_LIMITED_API), not(Py_3_10)))] +#[cfg(all(not(PyPy), not(Py_3_10)))] #[repr(C)] pub struct PyFunctionObject { pub ob_base: PyObject, @@ -22,7 +22,7 @@ pub struct PyFunctionObject { pub vectorcall: Option, } -#[cfg(all(not(PyPy), not(Py_LIMITED_API), Py_3_10))] +#[cfg(all(not(PyPy), Py_3_10))] #[repr(C)] pub struct PyFunctionObject { pub ob_base: PyObject, @@ -44,25 +44,24 @@ pub struct PyFunctionObject { pub func_version: u32, } -#[cfg(all(PyPy, not(Py_LIMITED_API)))] +#[cfg(PyPy)] #[repr(C)] pub struct PyFunctionObject { pub ob_base: PyObject, pub func_name: *mut PyObject, } -#[cfg(all(not(PyPy), Py_LIMITED_API))] -opaque_struct!(PyFunctionObject); - #[cfg_attr(windows, link(name = "pythonXY"))] extern "C" { + #[cfg(not(PyPy))] // broken, see https://foss.heptapod.net/pypy/pypy/-/issues/3776 #[cfg_attr(PyPy, link_name = "PyPyFunction_Type")] - pub static mut PyFunction_Type: PyTypeObject; + pub static mut PyFunction_Type: crate::PyTypeObject; } +#[cfg(not(PyPy))] #[inline] pub unsafe fn PyFunction_Check(op: *mut PyObject) -> c_int { - (Py_TYPE(op) == addr_of_mut_shim!(PyFunction_Type)) as c_int + (crate::Py_TYPE(op) == addr_of_mut_shim!(PyFunction_Type)) as c_int } extern "C" { diff --git a/pyo3-ffi/src/cpython/genobject.rs b/pyo3-ffi/src/cpython/genobject.rs index 4d494be3865..6b522bf8123 100644 --- a/pyo3-ffi/src/cpython/genobject.rs +++ b/pyo3-ffi/src/cpython/genobject.rs @@ -2,6 +2,8 @@ use crate::object::*; use crate::PyFrameObject; #[cfg(not(PyPy))] use crate::_PyErr_StackItem; +#[cfg(Py_3_11)] +use std::os::raw::c_char; use std::os::raw::c_int; #[cfg(not(PyPy))] @@ -9,6 +11,7 @@ use std::os::raw::c_int; #[derive(Copy, Clone)] pub struct PyGenObject { pub ob_base: PyObject, + #[cfg(not(Py_3_11))] pub gi_frame: *mut PyFrameObject, #[cfg(not(Py_3_10))] pub gi_running: c_int, @@ -17,6 +20,18 @@ pub struct PyGenObject { pub gi_name: *mut PyObject, pub gi_qualname: *mut PyObject, pub gi_exc_state: _PyErr_StackItem, + #[cfg(Py_3_11)] + pub gi_origin_or_finalizer: *mut PyObject, + #[cfg(Py_3_11)] + pub gi_hooks_inited: c_char, + #[cfg(Py_3_11)] + pub gi_closed: c_char, + #[cfg(Py_3_11)] + pub gi_running_async: c_char, + #[cfg(Py_3_11)] + pub gi_frame_state: i8, + #[cfg(Py_3_11)] + pub gi_iframe: [*mut PyObject; 1], } #[cfg_attr(windows, link(name = "pythonXY"))] diff --git a/pyo3-ffi/src/cpython/mod.rs b/pyo3-ffi/src/cpython/mod.rs index 068cedfb11d..b4623e6e74e 100644 --- a/pyo3-ffi/src/cpython/mod.rs +++ b/pyo3-ffi/src/cpython/mod.rs @@ -12,6 +12,7 @@ pub(crate) mod dictobject; // skipped fileobject.h // skipped fileutils.h pub(crate) mod frameobject; +pub(crate) mod funcobject; pub(crate) mod genobject; pub(crate) mod import; #[cfg(all(Py_3_8, not(PyPy)))] @@ -44,6 +45,7 @@ pub use self::descrobject::*; #[cfg(not(PyPy))] pub use self::dictobject::*; pub use self::frameobject::*; +pub use self::funcobject::*; pub use self::genobject::*; pub use self::import::*; #[cfg(all(Py_3_8, not(PyPy)))] diff --git a/pyo3-ffi/src/cpython/pystate.rs b/pyo3-ffi/src/cpython/pystate.rs index 2dcd6958019..a70af8e8542 100644 --- a/pyo3-ffi/src/cpython/pystate.rs +++ b/pyo3-ffi/src/cpython/pystate.rs @@ -31,8 +31,10 @@ pub const PyTrace_OPCODE: c_int = 7; #[repr(C)] #[derive(Clone, Copy)] pub struct _PyErr_StackItem { + #[cfg(not(Py_3_11))] pub exc_type: *mut PyObject, pub exc_value: *mut PyObject, + #[cfg(not(Py_3_11))] pub exc_traceback: *mut PyObject, pub previous_item: *mut _PyErr_StackItem, } diff --git a/pyo3-ffi/src/lib.rs b/pyo3-ffi/src/lib.rs index 1e3ee85596e..f37b758e857 100644 --- a/pyo3-ffi/src/lib.rs +++ b/pyo3-ffi/src/lib.rs @@ -299,8 +299,6 @@ pub use self::enumobject::*; pub use self::fileobject::*; pub use self::fileutils::*; pub use self::floatobject::*; -#[cfg(not(Py_LIMITED_API))] -pub use self::funcobject::*; pub use self::import::*; pub use self::intrcheck::*; pub use self::iterobject::*; @@ -368,8 +366,6 @@ mod fileobject; mod fileutils; mod floatobject; // skipped empty frameobject.h -#[cfg(not(Py_LIMITED_API))] -pub(crate) mod funcobject; // skipped genericaliasobject.h mod import; // skipped interpreteridobject.h diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index bccac283af7..3bf3e6f4471 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -339,19 +339,20 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec<'_>) -> MethodAndSlotDef { arg: *mut ::std::os::raw::c_void, ) -> ::std::os::raw::c_int { + let trap = _pyo3::impl_::panic::PanicTrap::new("uncaught panic inside __traverse__ handler"); let pool = _pyo3::GILPool::new(); let py = pool.python(); - _pyo3::callback::abort_on_traverse_panic(::std::panic::catch_unwind(move || { - let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf); - - let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py); - let borrow = slf.try_borrow(); - if let ::std::result::Result::Ok(borrow) = borrow { - _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit)) - } else { - 0 - } - })) + let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf); + + let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py); + let borrow = slf.try_borrow(); + let retval = if let ::std::result::Result::Ok(borrow) = borrow { + _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit)) + } else { + 0 + }; + trap.disarm(); + retval } }; let slot_def = quote! { diff --git a/src/callback.rs b/src/callback.rs index 62861f9ce34..457c9a53b38 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -5,6 +5,7 @@ use crate::err::{PyErr, PyResult}; use crate::exceptions::PyOverflowError; use crate::ffi::{self, Py_hash_t}; +use crate::impl_::panic::PanicTrap; use crate::panic::PanicException; use crate::{GILPool, IntoPyPointer}; use crate::{IntoPy, PyObject, Python}; @@ -243,9 +244,15 @@ where F: for<'py> FnOnce(Python<'py>) -> PyResult + UnwindSafe, R: PyCallbackOutput, { + let trap = PanicTrap::new("uncaught panic at ffi boundary"); let pool = GILPool::new(); let py = pool.python(); - panic_result_into_callback_output(py, panic::catch_unwind(move || -> PyResult<_> { body(py) })) + let out = panic_result_into_callback_output( + py, + panic::catch_unwind(move || -> PyResult<_> { body(py) }), + ); + trap.disarm(); + out } /// Converts the output of std::panic::catch_unwind into a Python function output, either by raising a Python @@ -259,28 +266,11 @@ pub fn panic_result_into_callback_output( where R: PyCallbackOutput, { - let py_result = match panic_result { - Ok(py_result) => py_result, - Err(payload) => Err(PanicException::from_panic_payload(payload)), + let py_err = match panic_result { + Ok(Ok(value)) => return value, + Ok(Err(py_err)) => py_err, + Err(payload) => PanicException::from_panic_payload(payload), }; - - py_result.unwrap_or_else(|py_err| { - py_err.restore(py); - R::ERR_VALUE - }) -} - -/// Aborts if panic has occurred. Used inside `__traverse__` implementations, where panicking is not possible. -#[doc(hidden)] -#[inline] -pub fn abort_on_traverse_panic( - panic_result: Result>, -) -> c_int { - match panic_result { - Ok(traverse_result) => traverse_result, - Err(_payload) => { - eprintln!("FATAL: panic inside __traverse__ handler; aborting."); - ::std::process::abort() - } - } + py_err.restore(py); + R::ERR_VALUE } diff --git a/src/err/mod.rs b/src/err/mod.rs index eb9b053d2c0..0c72918f329 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -463,11 +463,14 @@ impl PyErr { /// This is the opposite of `PyErr::fetch()`. #[inline] pub fn restore(self, py: Python<'_>) { - let (ptype, pvalue, ptraceback) = self - .state - .into_inner() - .expect("Cannot restore a PyErr while normalizing it") - .into_ffi_tuple(py); + let state = match self.state.into_inner() { + Some(state) => state, + // Safety: restore takes `self` by value so nothing else is accessing this err + // and the invariant is that state is always defined except during make_normalized + None => unsafe { std::hint::unreachable_unchecked() }, + }; + + let (ptype, pvalue, ptraceback) = state.into_ffi_tuple(py); unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) } } @@ -527,6 +530,11 @@ impl PyErr { } } + pub(crate) fn write_unraisable(self, py: Python<'_>, context: PyObject) { + self.restore(py); + unsafe { ffi::PyErr_WriteUnraisable(context.as_ptr()) }; + } + #[inline] fn from_state(state: PyErrState) -> PyErr { PyErr { diff --git a/src/gil.rs b/src/gil.rs index 45106d89748..bc1ec0e47a3 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -756,7 +756,7 @@ mod tests { let obj: Arc> = Arc::new(get_object(py)); let thread_obj = Arc::clone(&obj); - let count = (&*obj).get_refcnt(py); + let count = obj.get_refcnt(py); println!( "1: The object has been created and its reference count is {}", count diff --git a/src/impl_.rs b/src/impl_.rs index 64ca136062c..2481da35359 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -11,6 +11,7 @@ pub mod freelist; pub mod frompyobject; pub mod ghost; pub(crate) mod not_send; +pub mod panic; #[doc(hidden)] pub mod pyclass; #[doc(hidden)] diff --git a/src/impl_/panic.rs b/src/impl_/panic.rs new file mode 100644 index 00000000000..70cdab1ecee --- /dev/null +++ b/src/impl_/panic.rs @@ -0,0 +1,27 @@ +/// Type which will panic if dropped. +/// +/// If this is dropped during a panic, this will cause an abort. +/// +/// Use this to avoid letting unwinds cross through the FFI boundary, which is UB. +pub struct PanicTrap { + msg: &'static str, +} + +impl PanicTrap { + #[inline] + pub const fn new(msg: &'static str) -> Self { + Self { msg } + } + + #[inline] + pub const fn disarm(self) { + std::mem::forget(self) + } +} + +impl Drop for PanicTrap { + fn drop(&mut self) { + // Panic here will abort the process, assuming in an unwind. + panic!("{}", self.msg) + } +} diff --git a/src/panic.rs b/src/panic.rs index 4fe6100d626..50489d50aeb 100644 --- a/src/panic.rs +++ b/src/panic.rs @@ -16,7 +16,9 @@ Python interpreter to exit. ); impl PanicException { - // Try to format the error in the same way panic does + /// Creates a new PanicException from a panic payload. + /// + /// Attempts to format the error in the same way panic does. #[cold] pub(crate) fn from_panic_payload(payload: Box) -> PyErr { if let Some(string) = payload.downcast_ref::() { diff --git a/src/types/function.rs b/src/types/function.rs index 04438290201..d01b7ddadcb 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -1,11 +1,13 @@ use crate::derive_utils::PyFunctionArguments; use crate::exceptions::PyValueError; -use crate::prelude::*; +use crate::impl_::panic::PanicTrap; +use crate::panic::PanicException; use crate::{ ffi, impl_::pymethods::{self, PyMethodDef}, types, AsPyPointer, }; +use crate::{prelude::*, GILPool}; use std::os::raw::c_void; /// Represents a builtin Python function object. @@ -40,20 +42,20 @@ where F: Fn(&types::PyTuple, Option<&types::PyDict>) -> R + Send + 'static, R: crate::callback::IntoPyCallbackOutput<*mut ffi::PyObject>, { - let result = std::panic::catch_unwind(|| { + let trap = PanicTrap::new("uncaught panic during drop_closure"); + let pool = GILPool::new(); + if let Err(payload) = std::panic::catch_unwind(|| { let boxed_fn: Box = Box::from_raw(ffi::PyCapsule_GetPointer( capsule_ptr, CLOSURE_CAPSULE_NAME.as_ptr() as *const _, ) as *mut F); drop(boxed_fn) - }); - if let Err(err) = result { - // This second layer of catch_unwind is useful as eprintln! can also panic. - let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - eprintln!("--- PyO3 intercepted a panic when dropping a closure"); - eprintln!("{:?}", err); - })); - } + }) { + let py = pool.python(); + let err = PanicException::from_panic_payload(payload); + err.write_unraisable(py, "when dropping a closure".into_py(py)); + }; + trap.disarm(); } impl PyCFunction { @@ -166,7 +168,8 @@ impl PyCFunction { /// Represents a Python function object. #[repr(transparent)] +#[cfg(not(any(PyPy, Py_LIMITED_API)))] pub struct PyFunction(PyAny); -#[cfg(not(Py_LIMITED_API))] +#[cfg(not(any(PyPy, Py_LIMITED_API)))] pyobject_native_type_core!(PyFunction, ffi::PyFunction_Type, #checkfunction=ffi::PyFunction_Check); diff --git a/src/types/mod.rs b/src/types/mod.rs index f11e94a6386..cebf4f46ad3 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -20,7 +20,9 @@ pub use self::floatob::PyFloat; #[cfg(all(not(Py_LIMITED_API), not(PyPy)))] pub use self::frame::PyFrame; pub use self::frozenset::PyFrozenSet; -pub use self::function::{PyCFunction, PyFunction}; +pub use self::function::PyCFunction; +#[cfg(all(not(Py_LIMITED_API), not(PyPy)))] +pub use self::function::PyFunction; pub use self::iterator::PyIterator; pub use self::list::PyList; pub use self::mapping::PyMapping; diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 20fc67c5b2a..49dba14b61f 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -3,9 +3,11 @@ #[cfg(not(Py_LIMITED_API))] use pyo3::buffer::PyBuffer; use pyo3::prelude::*; -use pyo3::types::{self, PyCFunction}; #[cfg(not(Py_LIMITED_API))] -use pyo3::types::{PyDateTime, PyFunction}; +use pyo3::types::PyDateTime; +#[cfg(not(any(Py_LIMITED_API, PyPy)))] +use pyo3::types::PyFunction; +use pyo3::types::{self, PyCFunction}; mod common; @@ -70,7 +72,7 @@ assert a, array.array("i", [2, 4, 6, 8]) }); } -#[cfg(not(Py_LIMITED_API))] +#[cfg(not(any(Py_LIMITED_API, PyPy)))] #[pyfunction] fn function_with_pyfunction_arg(fun: &PyFunction) -> PyResult<&PyAny> { fun.call((), None) @@ -96,7 +98,7 @@ fn test_functions_with_function_args() { "# ); - #[cfg(not(Py_LIMITED_API))] + #[cfg(not(any(Py_LIMITED_API, PyPy)))] { let py_func_arg = wrap_pyfunction!(function_with_pyfunction_arg)(py).unwrap();