send errors in `__releasebuffer__` to `sys.unraisablehook`

This commit is contained in:
David Hewitt 2023-01-19 07:45:05 +00:00
parent bed4f9d6ee
commit 586fed2c4b
6 changed files with 186 additions and 69 deletions

View File

@ -362,7 +362,8 @@ Coercions:
### Buffer objects ### Buffer objects
- `__getbuffer__(<self>, *mut ffi::Py_buffer, flags) -> ()` - `__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 ### Garbage Collector Integration

View File

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

View File

@ -28,10 +28,6 @@ impl PyCallbackOutput for ffi::Py_ssize_t {
const ERR_VALUE: Self = -1; const ERR_VALUE: Self = -1;
} }
impl PyCallbackOutput for () {
const ERR_VALUE: Self = ();
}
/// Convert the result of callback function into the appropriate return value. /// Convert the result of callback function into the appropriate return value.
pub trait IntoPyCallbackOutput<Target> { pub trait IntoPyCallbackOutput<Target> {
fn convert(self, py: Python<'_>) -> PyResult<Target>; fn convert(self, py: Python<'_>) -> PyResult<Target>;

View File

@ -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 /// 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) /// (which is different to the rest of the trampolines which take py first)
#[inline] #[inline]
#[allow(clippy::unnecessary_wraps)] unsafe fn trampoline_dealloc_wrapper<T: PyClass>(py: Python<'_>, slf: *mut ffi::PyObject) {
unsafe fn trampoline_dealloc_wrapper<T: PyClass>(
py: Python<'_>,
slf: *mut ffi::PyObject,
) -> PyResult<()> {
T::Layout::tp_dealloc(slf, py); T::Layout::tp_dealloc(slf, py);
Ok(())
} }
// TODO change argument order in PyCellLayout::tp_dealloc so this wrapper isn't needed. // TODO change argument order in PyCellLayout::tp_dealloc so this wrapper isn't needed.
crate::impl_::trampoline::dealloc(obj, trampoline_dealloc_wrapper::<T>) crate::impl_::trampoline::dealloc(obj, trampoline_dealloc_wrapper::<T>)

View File

@ -140,15 +140,39 @@ trampolines!(
) -> *mut ffi::PyObject; ) -> *mut ffi::PyObject;
pub fn unaryfunc(slf: *mut ffi::PyObject) -> *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))] #[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 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 // Ipowfunc is a unique case where PyO3 has its own type
@ -201,3 +225,30 @@ where
py_err.restore(py); py_err.restore(py);
R::ERR_VALUE 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();
}

View File

@ -24,49 +24,11 @@ struct TestBufferClass {
#[pymethods] #[pymethods]
impl TestBufferClass { impl TestBufferClass {
unsafe fn __getbuffer__( unsafe fn __getbuffer__(
mut slf: PyRefMut<'_, Self>, slf: &PyCell<Self>,
view: *mut ffi::Py_buffer, view: *mut ffi::Py_buffer,
flags: c_int, flags: c_int,
) -> PyResult<()> { ) -> PyResult<()> {
if view.is_null() { fill_view_from_readonly_data(view, flags, &slf.borrow().vec, slf)
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(())
} }
unsafe fn __releasebuffer__(&self, view: *mut ffi::Py_buffer) { unsafe fn __releasebuffer__(&self, view: *mut ffi::Py_buffer) {
@ -86,7 +48,6 @@ impl Drop for TestBufferClass {
fn test_buffer() { fn test_buffer() {
let drop_called = Arc::new(AtomicBool::new(false)); let drop_called = Arc::new(AtomicBool::new(false));
{
Python::with_gil(|py| { Python::with_gil(|py| {
let instance = Py::new( let instance = Py::new(
py, py,
@ -99,7 +60,6 @@ fn test_buffer() {
let env = [("ob", instance)].into_py_dict(py); let env = [("ob", instance)].into_py_dict(py);
py_assert!(py, *env, "bytes(ob) == b' 23'"); py_assert!(py, *env, "bytes(ob) == b' 23'");
}); });
}
assert!(drop_called.load(Ordering::Relaxed)); assert!(drop_called.load(Ordering::Relaxed));
} }
@ -132,3 +92,116 @@ fn test_buffer_referenced() {
assert!(drop_called.load(Ordering::Relaxed)); assert!(drop_called.load(Ordering::Relaxed));
} }
#[test]
#[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8
fn test_releasebuffer_unraisable_error() {
use pyo3::exceptions::PyValueError;
#[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(())
}