Merge #2886

2886: send errors in `__releasebuffer__` to `sys.unraisablehook` r=davidhewitt a=davidhewitt

I noticed that in the `__releasebuffer__` implementation we currently diverge from documentation by allowing `-> PyResult<()>` return values, but they cause crashes later down the line.

e.g. without the other changes in this PR, the new test case would crash with the following message:

```
ValueError: oh dear

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
SystemError: <class 'bytes'> returned a result with an exception set
```

After some deliberation I decided to allow `-> PyResult<()>` returns because:
 - it keeps the macro implementation and usage simpler
 - errors might be produced by the `__releasebuffer__` internals anyway (e.g. due to `PyCell` locking, or invalid self type passed)

As a result, this PR cleans up the case discussed to send errors to `sys.unraisablehook`, and updates the documentation to be clearer on the allowed behaviour.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

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<()> {
+    unsafe fn trampoline_dealloc_wrapper<T: PyClass>(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::<T>)

src/impl_/trampoline.rs

@@ -140,15 +140,39 @@ 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_unraisable(|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_unraisable(
+        |py| {
+            f(py, slf);
+            Ok(())
+        },
+        std::ptr::null_mut(),
+    )
 }
 
 // Ipowfunc is a unique case where PyO3 has its own type
@@ -201,3 +225,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<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 = 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);
+        ffi::PyErr_WriteUnraisable(ctx);
+    }
+    trap.disarm();
+}

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<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 = 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<Self>,
+            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(())
+}