From aa0b5d84883c868f8e3ce51d6dabb2c20776d676 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 31 Mar 2021 22:37:41 +0100 Subject: [PATCH] buffer: tidy up exceptions --- CHANGELOG.md | 1 + src/buffer.rs | 111 +++++++++++++++++++-------------- tests/test_buffer.rs | 145 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 48 deletions(-) create mode 100644 tests/test_buffer.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fee84171..7fb289e6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/buffer.rs b/src/buffer.rs index d78e1427..71cbbf25 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -17,13 +17,15 @@ // DEALINGS IN THE SOFTWARE. //! `PyBuffer` implementation -use crate::err::{self, PyResult}; -use crate::{exceptions, ffi, AsPyPointer, FromPyObject, PyAny, PyNativeType, Python}; -use std::ffi::CStr; +use crate::{ + err, exceptions::PyBufferError, ffi, AsPyPointer, FromPyObject, PyAny, PyNativeType, PyResult, + 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 because Python expects that the Py_buffer struct has a stable memory address @@ -35,6 +37,24 @@ pub struct PyBuffer(Pin>, PhantomData); unsafe impl Send for PyBuffer {} unsafe impl Sync for PyBuffer {} +impl Debug for PyBuffer { + 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 { fn extract(obj: &PyAny) -> PyResult> { Self::get(obj) @@ -169,25 +176,37 @@ impl<'source, T: Element> FromPyObject<'source> for PyBuffer { impl PyBuffer { /// Get the underlying buffer from the specified python object. pub fn get(obj: &PyAny) -> PyResult> { - unsafe { - let mut buf = Box::pin(ffi::Py_buffer::new()); + // TODO: use nightly API Box::new_uninit() once stable + let mut buf = Box::new(mem::MaybeUninit::uninit()); + let buf: Box = 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)?; - let buf = PyBuffer(buf, PhantomData); - // Type Check - if mem::size_of::() == buf.item_size() - && (buf.0.buf as usize) % mem::align_of::() == 0 - && T::is_compatible_format(buf.format()) - { - Ok(buf) - } else { - Err(exceptions::PyBufferError::new_err( - "Incompatible type as buffer", - )) - } + // Safety: buf is initialized by PyObject_GetBuffer. + // TODO: use nightly API Box::assume_init() once stable + mem::transmute(buf) + }; + // Create PyBuffer immediately so that if validation checks fail, the PyBuffer::drop code + // will call PyBuffer_Release (thus avoiding any leaks). + let buf = PyBuffer(Pin::from(buf), PhantomData); + + if buf.0.shape.is_null() { + Err(PyBufferError::new_err("shape is null")) + } else if buf.0.strides.is_null() { + Err(PyBufferError::new_err("strides is null")) + } else if mem::size_of::() != buf.item_size() || !T::is_compatible_format(buf.format()) { + Err(PyBufferError::new_err(format!( + "buffer contents are not compatible with {}", + std::any::type_name::() + ))) + } else if buf.0.buf.align_offset(mem::align_of::()) != 0 { + Err(PyBufferError::new_err(format!( + "buffer contents are insufficiently aligned for {}", + std::any::type_name::() + ))) + } else { + Ok(buf) } } @@ -441,9 +460,11 @@ impl PyBuffer { 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( - "Slice length does not match buffer length.", - )); + return Err(PyBufferError::new_err(format!( + "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 PyBuffer { fn copy_from_slice_impl(&self, py: Python, source: &[T], fort: u8) -> PyResult<()> { if self.readonly() { - return buffer_readonly_error(); - } - if mem::size_of_val(source) != self.len_bytes() { - return Err(exceptions::PyBufferError::new_err( - "Slice length does not match buffer length.", - )); + return Err(PyBufferError::new_err("cannot write to read-only buffer")); + } else if mem::size_of_val(source) != self.len_bytes() { + return Err(PyBufferError::new_err(format!( + "slice to copy from (of length {}) does not match buffer length of {}", + source.len(), + self.item_count() + ))); } unsafe { err::error_on_minusone( @@ -562,13 +584,6 @@ impl PyBuffer { } } -#[inline(always)] -fn buffer_readonly_error() -> PyResult<()> { - Err(exceptions::PyBufferError::new_err( - "Cannot write to read-only buffer.", - )) -} - impl Drop for PyBuffer { fn drop(&mut self) { Python::with_gil(|_| unsafe { ffi::PyBuffer_Release(&mut *self.0) }); diff --git a/tests/test_buffer.rs b/tests/test_buffer.rs new file mode 100644 index 00000000..462cc556 --- /dev/null +++ b/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, + error: Option, +} + +#[pyproto] +impl PyBufferProtocol for TestBufferErrors { + fn bf_getbuffer(slf: PyRefMut, 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::() 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, _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::::get(instance.as_ref(py)).is_ok()); + + instance.borrow_mut(py).error = Some(TestGetBufferError::NullShape); + assert_eq!( + PyBuffer::::get(instance.as_ref(py)) + .unwrap_err() + .to_string(), + "BufferError: shape is null" + ); + + instance.borrow_mut(py).error = Some(TestGetBufferError::NullStrides); + assert_eq!( + PyBuffer::::get(instance.as_ref(py)) + .unwrap_err() + .to_string(), + "BufferError: strides is null" + ); + + instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectItemSize); + assert_eq!( + PyBuffer::::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::::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::::get(instance.as_ref(py)) + .unwrap_err() + .to_string(), + "BufferError: buffer contents are insufficiently aligned for u32" + ); + }); +}