Skip to content

Commit

Permalink
send errors in __releasebuffer__ to sys.unraisablehook
Browse files Browse the repository at this point in the history
  • Loading branch information
davidhewitt committed Jan 19, 2023
1 parent bed4f9d commit 0bc9e49
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 69 deletions.
3 changes: 2 additions & 1 deletion guide/src/class/protocols.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions newsfragments/2886.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Send errors returned by `__releasebuffer__` to `sys.unraisablehook` rather than causing `SystemError`.
4 changes: 0 additions & 4 deletions src/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
7 changes: 1 addition & 6 deletions src/impl_/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>)
Expand Down
59 changes: 55 additions & 4 deletions src/impl_/trampoline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

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| {
f(py, slf);
Ok(())
},
std::ptr::null_mut(),
)
}

// Ipowfunc is a unique case where PyO3 has its own type
Expand Down Expand Up @@ -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 = 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();
}
179 changes: 125 additions & 54 deletions tests/test_buffer_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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));
}
Expand Down Expand Up @@ -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(())
}

0 comments on commit 0bc9e49

Please sign in to comment.