buffer: tidy up exceptions

CHANGELOG.md

@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - The `auto-initialize` feature is no longer enabled by default. [#1443](https://github.com/PyO3/pyo3/pull/1443)
 - Change `PyCFunction::new()` and `PyCFunction::new_with_keywords()` to take `&'static str` arguments rather than implicitly copying (and leaking) them. [#1450](https://github.com/PyO3/pyo3/pull/1450)
 - Deprecate `PyModule` methods `call`, `call0`, `call1` and `get`. [#1492](https://github.com/PyO3/pyo3/pull/1492)
+- Add length information to `PyBufferError`s raised from `PyBuffer::copy_to_slice` and `PyBuffer::copy_from_slice`. [#1534](https://github.com/PyO3/pyo3/pull/1534)
 - Automatically provide `-undefined` and `dynamic_lookup` linker arguments on macOS with `extension-module` feature. [#1539](https://github.com/PyO3/pyo3/pull/1539)
 
 ### Removed

src/buffer.rs

@@ -17,13 +17,15 @@
 // DEALINGS IN THE SOFTWARE.
 
 //! `PyBuffer` implementation
-use crate::err::{self, PyResult};
+use crate::{
-use crate::{exceptions, ffi, AsPyPointer, FromPyObject, PyAny, PyNativeType, Python};
+    err, exceptions::PyBufferError, ffi, AsPyPointer, FromPyObject, PyAny, PyNativeType, PyResult,
-use std::ffi::CStr;
+    Python,
+};
 use std::marker::PhantomData;
 use std::os::raw;
 use std::pin::Pin;
 use std::{cell, mem, ptr, slice};
+use std::{ffi::CStr, fmt::Debug};
 
 /// Allows access to the underlying buffer used by a python object such as `bytes`, `bytearray` or `array.array`.
 // use Pin<Box> because Python expects that the Py_buffer struct has a stable memory address
@@ -35,6 +37,24 @@ pub struct PyBuffer<T>(Pin<Box<ffi::Py_buffer>>, PhantomData<T>);
 unsafe impl<T> Send for PyBuffer<T> {}
 unsafe impl<T> Sync for PyBuffer<T> {}
 
+impl<T> Debug for PyBuffer<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        f.debug_struct("PyBuffer")
+            .field("buf", &self.0.buf)
+            .field("obj", &self.0.obj)
+            .field("len", &self.0.len)
+            .field("itemsize", &self.0.itemsize)
+            .field("readonly", &self.0.readonly)
+            .field("ndim", &self.0.ndim)
+            .field("format", &self.0.format)
+            .field("shape", &self.0.shape)
+            .field("strides", &self.0.strides)
+            .field("suboffsets", &self.0.suboffsets)
+            .field("internal", &self.0.internal)
+            .finish()
+    }
+}
+
 #[derive(Copy, Clone, Eq, PartialEq)]
 pub enum ElementType {
     SignedInteger { bytes: usize },
@@ -147,19 +167,6 @@ pub unsafe trait Element: Copy {
     fn is_compatible_format(format: &CStr) -> bool;
 }
 
-fn validate(b: &ffi::Py_buffer) -> PyResult<()> {
-    // shape and stride information must be provided when we use PyBUF_FULL_RO
-    if b.shape.is_null() {
-        return Err(exceptions::PyBufferError::new_err("Shape is Null"));
-    }
-    if b.strides.is_null() {
-        return Err(exceptions::PyBufferError::new_err(
-            "PyBuffer: Strides is Null",
-        ));
-    }
-    Ok(())
-}
-
 impl<'source, T: Element> FromPyObject<'source> for PyBuffer<T> {
     fn extract(obj: &PyAny) -> PyResult<PyBuffer<T>> {
         Self::get(obj)
@@ -169,25 +176,37 @@ impl<'source, T: Element> FromPyObject<'source> for PyBuffer<T> {
 impl<T: Element> PyBuffer<T> {
     /// Get the underlying buffer from the specified python object.
     pub fn get(obj: &PyAny) -> PyResult<PyBuffer<T>> {
-        unsafe {
+        // TODO: use nightly API Box::new_uninit() once stable
-            let mut buf = Box::pin(ffi::Py_buffer::new());
+        let mut buf = Box::new(mem::MaybeUninit::uninit());
+        let buf: Box<ffi::Py_buffer> = unsafe {
             err::error_on_minusone(
                 obj.py(),
-                ffi::PyObject_GetBuffer(obj.as_ptr(), &mut *buf, ffi::PyBUF_FULL_RO),
+                ffi::PyObject_GetBuffer(obj.as_ptr(), buf.as_mut_ptr(), ffi::PyBUF_FULL_RO),
             )?;
-            validate(&buf)?;
+            // Safety: buf is initialized by PyObject_GetBuffer.
-            let buf = PyBuffer(buf, PhantomData);
+            // TODO: use nightly API Box::assume_init() once stable
-            // Type Check
+            mem::transmute(buf)
-            if mem::size_of::<T>() == buf.item_size()
+        };
-                && (buf.0.buf as usize) % mem::align_of::<T>() == 0
+        // Create PyBuffer immediately so that if validation checks fail, the PyBuffer::drop code
-                && T::is_compatible_format(buf.format())
+        // will call PyBuffer_Release (thus avoiding any leaks).
-            {
+        let buf = PyBuffer(Pin::from(buf), PhantomData);
-                Ok(buf)
+
-            } else {
+        if buf.0.shape.is_null() {
-                Err(exceptions::PyBufferError::new_err(
+            Err(PyBufferError::new_err("shape is null"))
-                    "Incompatible type as buffer",
+        } else if buf.0.strides.is_null() {
-                ))
+            Err(PyBufferError::new_err("strides is null"))
-            }
+        } else if mem::size_of::<T>() != buf.item_size() || !T::is_compatible_format(buf.format()) {
+            Err(PyBufferError::new_err(format!(
+                "buffer contents are not compatible with {}",
+                std::any::type_name::<T>()
+            )))
+        } else if buf.0.buf.align_offset(mem::align_of::<T>()) != 0 {
+            Err(PyBufferError::new_err(format!(
+                "buffer contents are insufficiently aligned for {}",
+                std::any::type_name::<T>()
+            )))
+        } else {
+            Ok(buf)
         }
     }
 
@@ -441,9 +460,11 @@ impl<T: Element> PyBuffer<T> {
 
     fn copy_to_slice_impl(&self, py: Python, target: &mut [T], fort: u8) -> PyResult<()> {
         if mem::size_of_val(target) != self.len_bytes() {
-            return Err(exceptions::PyBufferError::new_err(
+            return Err(PyBufferError::new_err(format!(
-                "Slice length does not match buffer length.",
+                "slice to copy to (of length {}) does not match buffer length of {}",
-            ));
+                target.len(),
+                self.item_count()
+            )));
         }
         unsafe {
             err::error_on_minusone(
@@ -525,12 +546,13 @@ impl<T: Element> PyBuffer<T> {
 
     fn copy_from_slice_impl(&self, py: Python, source: &[T], fort: u8) -> PyResult<()> {
         if self.readonly() {
-            return buffer_readonly_error();
+            return Err(PyBufferError::new_err("cannot write to read-only buffer"));
-        }
+        } else if mem::size_of_val(source) != self.len_bytes() {
-        if mem::size_of_val(source) != self.len_bytes() {
+            return Err(PyBufferError::new_err(format!(
-            return Err(exceptions::PyBufferError::new_err(
+                "slice to copy from (of length {}) does not match buffer length of {}",
-                "Slice length does not match buffer length.",
+                source.len(),
-            ));
+                self.item_count()
+            )));
         }
         unsafe {
             err::error_on_minusone(
@@ -562,13 +584,6 @@ impl<T: Element> PyBuffer<T> {
     }
 }
 
-#[inline(always)]
-fn buffer_readonly_error() -> PyResult<()> {
-    Err(exceptions::PyBufferError::new_err(
-        "Cannot write to read-only buffer.",
-    ))
-}
-
 impl<T> Drop for PyBuffer<T> {
     fn drop(&mut self) {
         Python::with_gil(|_| unsafe { ffi::PyBuffer_Release(&mut *self.0) });

tests/test_buffer.rs

@@ -0,0 +1,145 @@
+#![cfg(not(Py_LIMITED_API))]
+
+use pyo3::{
+    buffer::PyBuffer, class::PyBufferProtocol, exceptions::PyBufferError, ffi, prelude::*,
+    AsPyPointer,
+};
+use std::{
+    ffi::CStr,
+    os::raw::{c_int, c_void},
+    ptr,
+};
+
+#[macro_use]
+mod common;
+
+enum TestGetBufferError {
+    NullShape,
+    NullStrides,
+    IncorrectItemSize,
+    IncorrectFormat,
+    IncorrectAlignment,
+}
+
+#[pyclass]
+struct TestBufferErrors {
+    buf: Vec<u32>,
+    error: Option<TestGetBufferError>,
+}
+
+#[pyproto]
+impl PyBufferProtocol for TestBufferErrors {
+    fn bf_getbuffer(slf: PyRefMut<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"));
+        }
+
+        unsafe {
+            (*view).obj = slf.as_ptr();
+            ffi::Py_INCREF((*view).obj);
+        }
+
+        let bytes = &slf.buf;
+
+        unsafe {
+            (*view).buf = bytes.as_ptr() as *mut c_void;
+            (*view).len = bytes.len() as isize;
+            (*view).readonly = 1;
+            (*view).itemsize = std::mem::size_of::<u32>() as isize;
+
+            let msg = CStr::from_bytes_with_nul(b"I\0").unwrap();
+            (*view).format = msg.as_ptr() as *mut _;
+
+            (*view).ndim = 1;
+            (*view).shape = (&((*view).len)) as *const _ as *mut _;
+
+            (*view).strides = &((*view).itemsize) as *const _ as *mut _;
+
+            (*view).suboffsets = ptr::null_mut();
+            (*view).internal = ptr::null_mut();
+
+            if let Some(err) = &slf.error {
+                use TestGetBufferError::*;
+                match err {
+                    NullShape => {
+                        (*view).shape = std::ptr::null_mut();
+                    }
+                    NullStrides => {
+                        (*view).strides = std::ptr::null_mut();
+                    }
+                    IncorrectItemSize => {
+                        (*view).itemsize += 1;
+                    }
+                    IncorrectFormat => {
+                        (*view).format = CStr::from_bytes_with_nul(b"B\0").unwrap().as_ptr() as _;
+                    }
+                    IncorrectAlignment => (*view).buf = (*view).buf.add(1),
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    fn bf_releasebuffer(_slf: PyRefMut<Self>, _view: *mut ffi::Py_buffer) {}
+}
+
+#[test]
+fn test_get_buffer_errors() {
+    Python::with_gil(|py| {
+        let instance = Py::new(
+            py,
+            TestBufferErrors {
+                buf: vec![0, 1, 2, 3],
+                error: None,
+            },
+        )
+        .unwrap();
+
+        assert!(PyBuffer::<u32>::get(instance.as_ref(py)).is_ok());
+
+        instance.borrow_mut(py).error = Some(TestGetBufferError::NullShape);
+        assert_eq!(
+            PyBuffer::<u32>::get(instance.as_ref(py))
+                .unwrap_err()
+                .to_string(),
+            "BufferError: shape is null"
+        );
+
+        instance.borrow_mut(py).error = Some(TestGetBufferError::NullStrides);
+        assert_eq!(
+            PyBuffer::<u32>::get(instance.as_ref(py))
+                .unwrap_err()
+                .to_string(),
+            "BufferError: strides is null"
+        );
+
+        instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectItemSize);
+        assert_eq!(
+            PyBuffer::<u32>::get(instance.as_ref(py))
+                .unwrap_err()
+                .to_string(),
+            "BufferError: buffer contents are not compatible with u32"
+        );
+
+        instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectFormat);
+        assert_eq!(
+            PyBuffer::<u32>::get(instance.as_ref(py))
+                .unwrap_err()
+                .to_string(),
+            "BufferError: buffer contents are not compatible with u32"
+        );
+
+        instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectAlignment);
+        assert_eq!(
+            PyBuffer::<u32>::get(instance.as_ref(py))
+                .unwrap_err()
+                .to_string(),
+            "BufferError: buffer contents are insufficiently aligned for u32"
+        );
+    });
+}