From 6b31ed71ee4b8237b6b767c0423ec2dad3650295 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 18 Jan 2023 08:47:50 +0000 Subject: [PATCH] send errors in `__releasebuffer__` to `sys.unraisablehook` --- guide/src/class/protocols.md | 3 +- newsfragments/2886.fixed.md | 1 + src/callback.rs | 4 - src/impl_/pyclass.rs | 9 +- src/impl_/trampoline.rs | 53 +++++++++++- tests/test_buffer_protocol.rs | 119 +++++++++++++++++++++++---- tests/ui/invalid_pymethods_buffer.rs | 18 +++- 7 files changed, 176 insertions(+), 31 deletions(-) create mode 100644 newsfragments/2886.fixed.md diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index c269eb3ade2..5f22c157902 100644 --- a/guide/src/class/protocols.md +++ b/guide/src/class/protocols.md @@ -362,7 +362,8 @@ Coercions: ### Buffer objects - `__getbuffer__(, *mut ffi::Py_buffer, flags) -> ()` - - `__releasebuffer__(, *mut ffi::Py_buffer)` (no return value, not even `PyResult`) + - `__releasebuffer__(, *mut ffi::Py_buffer) -> ()` + Errors returned from `__releasebuffer__` will be sent to `sys.unraiseablehook`. It is strongly advised to never return an error from `__releasebuffer__`, and if it really is necessary, to make best effort to perform any required freeing operations before returning. `__releasebuffer__` will not be called a second time; anything not freed will be leaked. ### Garbage Collector Integration diff --git a/newsfragments/2886.fixed.md b/newsfragments/2886.fixed.md new file mode 100644 index 00000000000..cfe192d4ef0 --- /dev/null +++ b/newsfragments/2886.fixed.md @@ -0,0 +1 @@ +Send errors returned by `__releasebuffer__` to `sys.unraisablehook` rather than causing `SystemError`. diff --git a/src/callback.rs b/src/callback.rs index 588639bbd67..6d59253730e 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -28,10 +28,6 @@ impl PyCallbackOutput for ffi::Py_ssize_t { const ERR_VALUE: Self = -1; } -impl PyCallbackOutput for () { - const ERR_VALUE: Self = (); -} - /// Convert the result of callback function into the appropriate return value. pub trait IntoPyCallbackOutput { fn convert(self, py: Python<'_>) -> PyResult; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index b7d400112ff..3b16fda4e8e 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -931,13 +931,8 @@ pub(crate) unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) /// A wrapper because PyCellLayout::tp_dealloc currently takes the py argument last /// (which is different to the rest of the trampolines which take py first) #[inline] - #[allow(clippy::unnecessary_wraps)] - unsafe fn trampoline_dealloc_wrapper( - py: Python<'_>, - slf: *mut ffi::PyObject, - ) -> PyResult<()> { - T::Layout::tp_dealloc(slf, py); - Ok(()) + unsafe fn trampoline_dealloc_wrapper(py: Python<'_>, slf: *mut ffi::PyObject) { + T::Layout::tp_dealloc(slf, py) } // TODO change argument order in PyCellLayout::tp_dealloc so this wrapper isn't needed. crate::impl_::trampoline::dealloc(obj, trampoline_dealloc_wrapper::) diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs index 44227fa78fe..c8363afd552 100644 --- a/src/impl_/trampoline.rs +++ b/src/impl_/trampoline.rs @@ -140,15 +140,33 @@ trampolines!( ) -> *mut ffi::PyObject; pub fn unaryfunc(slf: *mut ffi::PyObject) -> *mut ffi::PyObject; - - pub fn dealloc(slf: *mut ffi::PyObject) -> (); ); #[cfg(any(not(Py_LIMITED_API), Py_3_11))] -trampolines! { +trampoline! { pub fn getbufferproc(slf: *mut ffi::PyObject, buf: *mut ffi::Py_buffer, flags: c_int) -> c_int; +} + +#[cfg(any(not(Py_LIMITED_API), Py_3_11))] +#[inline] +pub unsafe fn releasebufferproc( + slf: *mut ffi::PyObject, + buf: *mut ffi::Py_buffer, + f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::Py_buffer) -> PyResult<()>, +) { + trampoline_inner_void(|py| f(py, slf, buf), slf) +} - pub fn releasebufferproc(slf: *mut ffi::PyObject, buf: *mut ffi::Py_buffer) -> (); +#[inline] +pub(crate) unsafe fn dealloc( + slf: *mut ffi::PyObject, + f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> (), +) { + // After calling tp_dealloc the object is no longer valid, + // so pass null_mut() to the context. + // + // (Note that we don't allow the implementation `f` to fail.) + trampoline_inner_void(|py| Ok(f(py, slf)), std::ptr::null_mut()) } // Ipowfunc is a unique case where PyO3 has its own type @@ -201,3 +219,30 @@ where py_err.restore(py); R::ERR_VALUE } + +/// Implementation of trampoline for functions which can't return an error. +/// +/// Panics during execution are trapped so that they don't propagate through any +/// outer FFI boundary. +/// +/// Exceptions produced are sent to `sys.unraisablehook`. +/// +/// # Safety +/// +/// ctx must be either a valid ffi::PyObject or NULL +unsafe fn trampoline_inner_void(body: F, ctx: *mut ffi::PyObject) +where + F: for<'py> FnOnce(Python<'py>) -> PyResult<()> + UnwindSafe, +{ + let trap = PanicTrap::new("uncaught panic at ffi boundary"); + let pool = unsafe { GILPool::new() }; + let py = pool.python(); + if let Err(py_err) = match panic::catch_unwind(move || body(py)) { + Ok(inner) => inner, + Err(payload) => Err(PanicException::from_panic_payload(payload)), + } { + py_err.restore(py); + unsafe { ffi::PyErr_WriteUnraisable(ctx) }; + } + trap.disarm(); +} diff --git a/tests/test_buffer_protocol.rs b/tests/test_buffer_protocol.rs index c7d2a399a01..ab432d256b1 100644 --- a/tests/test_buffer_protocol.rs +++ b/tests/test_buffer_protocol.rs @@ -1,8 +1,11 @@ #![cfg(feature = "macros")] #![cfg(any(not(Py_LIMITED_API), Py_3_11))] +use parking_lot::const_mutex; +use parking_lot::Mutex; use pyo3::buffer::PyBuffer; use pyo3::exceptions::PyBufferError; +use pyo3::exceptions::PyValueError; use pyo3::ffi; use pyo3::prelude::*; use pyo3::types::IntoPyDict; @@ -86,20 +89,18 @@ impl Drop for TestBufferClass { fn test_buffer() { let drop_called = Arc::new(AtomicBool::new(false)); - { - Python::with_gil(|py| { - let instance = Py::new( - py, - TestBufferClass { - vec: vec![b' ', b'2', b'3'], - drop_called: drop_called.clone(), - }, - ) - .unwrap(); - let env = [("ob", instance)].into_py_dict(py); - py_assert!(py, *env, "bytes(ob) == b' 23'"); - }); - } + Python::with_gil(|py| { + let instance = Py::new( + py, + TestBufferClass { + vec: vec![b' ', b'2', b'3'], + drop_called: drop_called.clone(), + }, + ) + .unwrap(); + let env = [("ob", instance)].into_py_dict(py); + py_assert!(py, *env, "bytes(ob) == b' 23'"); + }); assert!(drop_called.load(Ordering::Relaxed)); } @@ -132,3 +133,93 @@ fn test_buffer_referenced() { assert!(drop_called.load(Ordering::Relaxed)); } + +#[pyclass] +struct ReleaseBufferError {} + +static BUF_BYTES: &[u8] = b"hello world"; + +#[pymethods] +impl ReleaseBufferError { + unsafe fn __getbuffer__( + slf: PyRef<'_, Self>, + view: *mut ffi::Py_buffer, + flags: c_int, + ) -> PyResult<()> { + if view.is_null() { + return Err(PyBufferError::new_err("View is null")); + } + + if (flags & ffi::PyBUF_WRITABLE) == ffi::PyBUF_WRITABLE { + return Err(PyBufferError::new_err("Object is not writable")); + } + + (*view).obj = ffi::_Py_NewRef(slf.as_ptr()); + + (*view).buf = BUF_BYTES.as_ptr() as *mut c_void; + (*view).len = BUF_BYTES.len() as isize; + (*view).readonly = 1; + (*view).itemsize = 1; + + (*view).format = if (flags & ffi::PyBUF_FORMAT) == ffi::PyBUF_FORMAT { + let msg = CString::new("B").unwrap(); + msg.into_raw() + } else { + ptr::null_mut() + }; + + (*view).ndim = 1; + (*view).shape = if (flags & ffi::PyBUF_ND) == ffi::PyBUF_ND { + &mut (*view).len + } else { + ptr::null_mut() + }; + + (*view).strides = if (flags & ffi::PyBUF_STRIDES) == ffi::PyBUF_STRIDES { + &mut (*view).itemsize + } else { + ptr::null_mut() + }; + + (*view).suboffsets = ptr::null_mut(); + (*view).internal = ptr::null_mut(); + + Ok(()) + } + + unsafe fn __releasebuffer__(&self, _view: *mut ffi::Py_buffer) -> PyResult<()> { + Err(PyValueError::new_err("oh dear")) + } +} + +#[test] +fn test_releasebuffer_unraisable_error() { + static UNRAISABLE_CALLED: Mutex> = const_mutex(None); + + #[pyfunction] + fn unraisablehook(unraisable: &PyAny) { + let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap()); + let instance = unraisable.getattr("object").unwrap(); + *UNRAISABLE_CALLED.lock() = Some((err, instance.into())); + } + + Python::with_gil(|py| { + let sys = py.import("sys").unwrap(); + let old_hook = sys.getattr("unraisablehook").unwrap(); + let hook = wrap_pyfunction!(unraisablehook, py).unwrap(); + sys.setattr("unraisablehook", hook).unwrap(); + + let instance = Py::new(py, ReleaseBufferError {}).unwrap(); + let env = [("ob", instance.clone())].into_py_dict(py); + + assert!(UNRAISABLE_CALLED.lock().is_none()); + + py_assert!(py, *env, "bytes(ob) == b'hello world'"); + + let (err, object) = UNRAISABLE_CALLED.lock().take().unwrap(); + assert_eq!(err.to_string(), "ValueError: oh dear"); + assert!(object.is(&instance)); + + sys.setattr("unraisablehook", old_hook).unwrap(); + }); +} diff --git a/tests/ui/invalid_pymethods_buffer.rs b/tests/ui/invalid_pymethods_buffer.rs index 4eb7c28e091..257e10405ed 100644 --- a/tests/ui/invalid_pymethods_buffer.rs +++ b/tests/ui/invalid_pymethods_buffer.rs @@ -6,7 +6,12 @@ struct MyClass {} #[pymethods] impl MyClass { #[pyo3(name = "__getbuffer__")] - fn getbuffer_must_be_unsafe(&self, _view: *mut pyo3::ffi::Py_buffer, _flags: std::os::raw::c_int) {} + fn getbuffer_must_be_unsafe( + &self, + _view: *mut pyo3::ffi::Py_buffer, + _flags: std::os::raw::c_int, + ) { + } } #[pymethods] @@ -15,4 +20,15 @@ impl MyClass { fn releasebuffer_must_be_unsafe(&self, _view: *mut pyo3::ffi::Py_buffer) {} } +#[pymethods] +impl MyClass { + #[pyo3(name = "__releasebuffer__")] + pub fn releasebuffer_cannot_return_error( + &self, + _view: *mut pyo3::ffi::Py_buffer, + ) -> PyResult<()> { + Ok(()) + } +} + fn main() {}