send errors in __releasebuffer__ to sys.unraisablehook

guide/src/class/protocols.md

@@ -362,7 +362,8 @@ Coercions:
 ### Buffer objects
 
   - `__getbuffer__(<self>, *mut ffi::Py_buffer, flags) -> ()`
-  - `__releasebuffer__(<self>, *mut ffi::Py_buffer)` (no return value, not even `PyResult`)
+  - `__releasebuffer__(<self>, *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
 

newsfragments/2886.fixed.md

@@ -0,0 +1 @@
+Send errors returned by `__releasebuffer__` to `sys.unraisablehook` rather than causing `SystemError`.

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<Target> {
     fn convert(self, py: Python<'_>) -> PyResult<Target>;

src/impl_/pyclass.rs

@@ -931,13 +931,8 @@ pub(crate) unsafe extern "C" fn tp_dealloc<T: PyClass>(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<T: PyClass>(
-        py: Python<'_>,
-        slf: *mut ffi::PyObject,
-    ) -> PyResult<()> {
-        T::Layout::tp_dealloc(slf, py);
-        Ok(())
+    unsafe fn trampoline_dealloc_wrapper<T: PyClass>(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::<T>)

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<F>(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();
+}

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<Option<(PyErr, PyObject)>> = 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();
+    });
+}

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() {}