From 6dfc28dd3be2b5b092a58c07849fb0421a3159ab Mon Sep 17 00:00:00 2001 From: ginnyTheCat Date: Tue, 18 Feb 2025 21:34:02 +0100 Subject: [PATCH] Add safety comment and error handling --- mupdf-sys/src/lib.rs | 56 ++++++++++++++++++++++++++++++++++++++++---- mupdf-sys/wrapper.c | 14 +++++++++++ src/device.rs | 2 +- src/device/native.rs | 18 +++++++------- 4 files changed, 77 insertions(+), 13 deletions(-) diff --git a/mupdf-sys/src/lib.rs b/mupdf-sys/src/lib.rs index e794ec3..7704dc7 100644 --- a/mupdf-sys/src/lib.rs +++ b/mupdf-sys/src/lib.rs @@ -5,12 +5,60 @@ include!(concat!(env!("OUT_DIR"), "/bindings.rs")); +use core::{ + ffi::{c_int, CStr}, + ptr, +}; + +/// This function allocates a new device and returns a pointer to it if no error occured. For the +/// required structure of `T` check the example below. +/// +/// The pointer `errptr` points to is expected to be null at the time this function is called. +/// If an error occurs this inner pointer will be set to the error and null returned from this +/// function. Was this inner pointer non-null before this function was called a new device will be +/// allocated but the function will assume that an error has occured and will return null. This device +/// will not be freed. It will, however, not cause unsafe behavior either. +/// +/// # Safety +/// +/// The caller must ensure `ctx` and `errptr` to be a valid pointers. +/// +/// It must also ensure `T` to be a type that starts with `fz_device`. Memory will be allocated for +/// a new instance of `T`, but only the `fz_device` portion will be initialized. The rest is +/// currently being zero-initialized, but this might change in the future. +/// +/// # Example +/// +/// This is how a compliant `T` might look like. The `repr(C)` is necessary as `repr(Rust)` does +/// not guarantee stable field orderings. +/// +/// ```rust +/// use mupdf_sys::fz_device; +/// +/// #[repr(C)] +/// struct MyDevice { +/// base: fz_device, +/// foo: u32, +/// } +/// ``` pub unsafe fn mupdf_new_derived_device( ctx: *mut fz_context, - label: *const std::ffi::c_char, + label: &'static CStr, + errptr: *mut *mut mupdf_error_t, ) -> *mut T { - let size = std::ffi::c_int::try_from(size_of::()).unwrap(); - let device = fz_new_device_of_size(ctx, size); - let label = Memento_label(device.cast(), label); + let SIZE: c_int = const { + if (c_int::MAX as usize) < size_of::() { + panic!("device too big") + } else { + size_of::() as c_int + } + }; + + let device = mupdf_new_device_of_size(ctx, SIZE, errptr); + if !(*errptr).is_null() { + return ptr::null_mut(); + } + + let label = Memento_label(device.cast(), label.as_ptr()); label.cast() } diff --git a/mupdf-sys/wrapper.c b/mupdf-sys/wrapper.c index 952f63d..ab70349 100644 --- a/mupdf-sys/wrapper.c +++ b/mupdf-sys/wrapper.c @@ -2640,6 +2640,20 @@ fz_device *mupdf_new_draw_device(fz_context *ctx, fz_pixmap *pixmap, fz_irect cl return device; } +fz_device *mupdf_new_device_of_size(fz_context *ctx, int size, mupdf_error_t **errptr) +{ + fz_device *device = NULL; + fz_try(ctx) + { + device = fz_new_device_of_size(ctx, size); + } + fz_catch(ctx) + { + mupdf_save_error(ctx, errptr); + } + return device; +} + fz_device *mupdf_new_display_list_device(fz_context *ctx, fz_display_list *list, mupdf_error_t **errptr) { fz_device *device = NULL; diff --git a/src/device.rs b/src/device.rs index 488dee7..eeaa2e2 100644 --- a/src/device.rs +++ b/src/device.rs @@ -182,7 +182,7 @@ impl Device { Self { dev, list } } - pub fn from_native(device: D) -> Self { + pub fn from_native(device: D) -> Result { native::create(device) } diff --git a/src/device/native.rs b/src/device/native.rs index fd2829d..7780b96 100644 --- a/src/device/native.rs +++ b/src/device/native.rs @@ -8,8 +8,8 @@ use std::{ use mupdf_sys::*; use crate::{ - context, BlendMode, ColorParams, Colorspace, Device, Function, Image, Matrix, Path, Rect, - Shade, StrokeState, Text, + context, BlendMode, ColorParams, Colorspace, Device, Error, Function, Image, Matrix, Path, + Rect, Shade, StrokeState, Text, }; use super::{DefaultColorspaces, DeviceFlag, Metatext, Structure}; @@ -360,9 +360,10 @@ impl NativeDevice for &mut T { } } -pub(crate) fn create(device: D) -> Device { - unsafe { - let c_device = mupdf_new_derived_device::>(context(), c"RustDevice".as_ptr()); +pub(crate) fn create(device: D) -> Result { + let ret = unsafe { + let c_device: *mut CDevice = + ffi_try!(mupdf_new_derived_device(context(), c"RustDevice")); ptr::write(&raw mut (*c_device).rust_device, device); (*c_device).base.close_device = Some(close_device::); @@ -407,7 +408,8 @@ pub(crate) fn create(device: D) -> Device { (*c_device).base.end_metatext = Some(end_metatext::); Device::from_raw(c_device.cast(), ptr::null_mut()) - } + }; + Ok(ret) } #[repr(C)] @@ -956,7 +958,7 @@ mod test { } let mut ndev = Test::default(); - let dev = Device::from_native(&mut ndev); + let dev = Device::from_native(&mut ndev).unwrap(); dev.begin_layer("begin layer name").unwrap(); assert_eq!(ndev.begin_layer, 1); @@ -984,7 +986,7 @@ mod test { let detector = proof.clone(); assert_eq!(Rc::strong_count(&proof), 2, "setup failed"); - let dev = Device::from_native(DropDevice(detector)); + let dev = Device::from_native(DropDevice(detector)).unwrap(); assert_eq!(Rc::strong_count(&proof), 2, "dropped too early"); drop(dev); assert_eq!(Rc::strong_count(&proof), 1, "not dropped");