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..6d1a93732f4 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<()> { + unsafe fn trampoline_dealloc_wrapper(py: Python<'_>, slf: *mut ffi::PyObject) { T::Layout::tp_dealloc(slf, py); - Ok(()) } // 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..29932804930 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; +} - pub fn releasebufferproc(slf: *mut ffi::PyObject, buf: *mut ffi::Py_buffer) -> (); +#[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_unraisable(|py| f(py, slf, buf), slf) +} + +#[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_unraisable(|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 +#[inline] +unsafe fn trampoline_inner_unraisable(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) = panic::catch_unwind(move || body(py)) + .unwrap_or_else(|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..5217254f81a 100644 --- a/tests/test_buffer_protocol.rs +++ b/tests/test_buffer_protocol.rs @@ -3,6 +3,7 @@ use pyo3::buffer::PyBuffer; use pyo3::exceptions::PyBufferError; +use pyo3::exceptions::PyValueError; use pyo3::ffi; use pyo3::prelude::*; use pyo3::types::IntoPyDict; @@ -24,49 +25,11 @@ struct TestBufferClass { #[pymethods] impl TestBufferClass { unsafe fn __getbuffer__( - mut slf: PyRefMut<'_, Self>, + slf: &PyCell, 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 = slf.vec.as_mut_ptr() as *mut c_void; - (*view).len = slf.vec.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(()) + fill_view_from_readonly_data(view, flags, &slf.borrow().vec, slf) } unsafe fn __releasebuffer__(&self, view: *mut ffi::Py_buffer) { @@ -86,20 +49,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 +93,113 @@ fn test_buffer_referenced() { assert!(drop_called.load(Ordering::Relaxed)); } + +#[test] +fn test_releasebuffer_unraisable_error() { + #[pyclass] + struct ReleaseBufferError {} + + #[pymethods] + impl ReleaseBufferError { + unsafe fn __getbuffer__( + slf: &PyCell, + view: *mut ffi::Py_buffer, + flags: c_int, + ) -> PyResult<()> { + static BUF_BYTES: &[u8] = b"hello world"; + fill_view_from_readonly_data(view, flags, BUF_BYTES, slf) + } + + unsafe fn __releasebuffer__(&self, _view: *mut ffi::Py_buffer) -> PyResult<()> { + Err(PyValueError::new_err("oh dear")) + } + } + + #[pyclass] + struct UnraisableCapture { + capture: Option<(PyErr, PyObject)>, + } + + #[pymethods] + impl UnraisableCapture { + fn hook(&mut self, unraisable: &PyAny) { + let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap()); + let instance = unraisable.getattr("object").unwrap(); + self.capture = Some((err, instance.into())); + } + } + + Python::with_gil(|py| { + let sys = py.import("sys").unwrap(); + let old_hook = sys.getattr("unraisablehook").unwrap(); + let capture = Py::new(py, UnraisableCapture { capture: None }).unwrap(); + + sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap()) + .unwrap(); + + let instance = Py::new(py, ReleaseBufferError {}).unwrap(); + let env = [("ob", instance.clone())].into_py_dict(py); + + assert!(capture.borrow(py).capture.is_none()); + + py_assert!(py, *env, "bytes(ob) == b'hello world'"); + + let (err, object) = capture.borrow_mut(py).capture.take().unwrap(); + assert_eq!(err.to_string(), "ValueError: oh dear"); + assert!(object.is(&instance)); + + sys.setattr("unraisablehook", old_hook).unwrap(); + }); +} + +/// # Safety +/// +/// `view` must be a valid pointer to ffi::Py_buffer, or null +/// `data` must outlive the Python lifetime of `owner` (i.e. data must be owned by owner, or data +/// must be static data) +unsafe fn fill_view_from_readonly_data( + view: *mut ffi::Py_buffer, + flags: c_int, + data: &[u8], + owner: &PyAny, +) -> 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(owner.as_ptr()); + + (*view).buf = data.as_ptr() as *mut c_void; + (*view).len = data.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(()) +}