From e9fd31a77bf961dc1f7ecc746550e8deda9bdb57 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 12 May 2019 18:48:09 +0200 Subject: [PATCH 1/3] Add a safe API for user data --- cairo-sys-rs/src/lib.rs | 20 ++++---- src/device.rs | 6 +++ src/font/font_face.rs | 6 +++ src/font/scaled_font.rs | 6 +++ src/lib.rs | 3 ++ src/patterns.rs | 16 +++++++ src/surface.rs | 6 +++ src/user_data.rs | 103 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 src/user_data.rs diff --git a/cairo-sys-rs/src/lib.rs b/cairo-sys-rs/src/lib.rs index 649a4026..426dc78a 100644 --- a/cairo-sys-rs/src/lib.rs +++ b/cairo-sys-rs/src/lib.rs @@ -431,8 +431,8 @@ extern "C" { pub fn cairo_pattern_get_matrix(pattern: *mut cairo_pattern_t, matrix: *mut Matrix); pub fn cairo_pattern_get_type(pattern: *mut cairo_pattern_t) -> cairo_pattern_type_t; pub fn cairo_pattern_get_reference_count(pattern: *mut cairo_pattern_t) -> c_uint; - //pub fn cairo_pattern_set_user_data(pattern: *mut cairo_pattern_t, key: *mut cairo_user_data_key_t, user_data: *mut void, destroy: cairo_destroy_func_t) -> cairo_status_t; - //pub fn cairo_pattern_get_user_data(pattern: *mut cairo_pattern_t, key: *mut cairo_user_data_key_t) -> *mut void; + pub fn cairo_pattern_set_user_data(pattern: *mut cairo_pattern_t, key: *const cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; + pub fn cairo_pattern_get_user_data(pattern: *mut cairo_pattern_t, key: *const cairo_user_data_key_t) -> *mut c_void; // CAIRO REGIONS pub fn cairo_region_create() -> *mut cairo_region_t; @@ -519,8 +519,8 @@ extern "C" { pub fn cairo_font_face_status(font_face: *mut cairo_font_face_t) -> cairo_status_t; pub fn cairo_font_face_get_type(font_face: *mut cairo_font_face_t) -> cairo_font_type_t; pub fn cairo_font_face_get_reference_count(font_face: *mut cairo_font_face_t) -> c_uint; - //pub fn cairo_font_face_set_user_data(font_face: *mut cairo_font_face_t, key: *mut cairo_user_data_key_t, user_data: *mut void, destroy: cairo_destroy_func_t) -> cairo_status_t; - //pub fn cairo_font_face_get_user_data(font_face: *mut cairo_font_face_t, key: *mut cairo_user_data_key_t) -> *mut void; + pub fn cairo_font_face_set_user_data(font_face: *mut cairo_font_face_t, key: *const cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; + pub fn cairo_font_face_get_user_data(font_face: *mut cairo_font_face_t, key: *const cairo_user_data_key_t) -> *mut c_void; // CAIRO SCALED FONT pub fn cairo_scaled_font_create(font_face: *mut cairo_font_face_t, font_matrix: *const Matrix, ctm: *const Matrix, options: *const cairo_font_options_t) -> *mut cairo_scaled_font_t; @@ -538,8 +538,8 @@ extern "C" { pub fn cairo_scaled_font_get_scale_matrix(scaled_font: *mut cairo_scaled_font_t, scale_matrix: *mut Matrix); pub fn cairo_scaled_font_get_type(scaled_font: *mut cairo_scaled_font_t) -> cairo_font_type_t; pub fn cairo_scaled_font_get_reference_count(font_face: *mut cairo_scaled_font_t) -> c_uint; - //pub fn cairo_scaled_font_set_user_data(scaled_font: *mut cairo_scaled_font_t, key: *mut cairo_user_data_key_t, user_data: *mut void, destroy: cairo_destroy_func_t) -> cairo_status_t; - //pub fn cairo_scaled_font_get_user_data(scaled_font: *mut cairo_scaled_font_t, key: *mut cairo_user_data_key_t) -> *mut void; + pub fn cairo_scaled_font_set_user_data(scaled_font: *mut cairo_scaled_font_t, key: *const cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; + pub fn cairo_scaled_font_get_user_data(scaled_font: *mut cairo_scaled_font_t, key: *const cairo_user_data_key_t) -> *mut c_void; // CAIRO FONT OPTIONS pub fn cairo_font_options_create() -> *mut cairo_font_options_t; @@ -580,8 +580,8 @@ extern "C" { pub fn cairo_surface_status(surface: *mut cairo_surface_t) -> cairo_status_t; pub fn cairo_surface_get_type(surface: *mut cairo_surface_t) -> cairo_surface_type_t; pub fn cairo_surface_reference(surface: *mut cairo_surface_t) -> *mut cairo_surface_t; - pub fn cairo_surface_get_user_data(surface: *mut cairo_surface_t, key: *mut cairo_user_data_key_t) -> *mut c_void; - pub fn cairo_surface_set_user_data(surface: *mut cairo_surface_t, key: *mut cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; + pub fn cairo_surface_get_user_data(surface: *mut cairo_surface_t, key: *const cairo_user_data_key_t) -> *mut c_void; + pub fn cairo_surface_set_user_data(surface: *mut cairo_surface_t, key: *const cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; pub fn cairo_surface_get_reference_count(surface: *mut cairo_surface_t) -> c_uint; pub fn cairo_surface_mark_dirty(surface: *mut cairo_surface_t); pub fn cairo_surface_create_similar(surface: *mut cairo_surface_t, content: cairo_content_t, width: c_int, height: c_int) -> *mut cairo_surface_t; @@ -862,8 +862,8 @@ extern "C" { pub fn cairo_device_get_type(device: *mut cairo_device_t) -> cairo_device_type_t; pub fn cairo_device_reference(device: *mut cairo_device_t) -> *mut cairo_device_t; pub fn cairo_device_get_reference_count(device: *mut cairo_device_t) -> c_uint; - // pub fn cairo_device_set_user_data() -> cairo_status_t; - // pub fn cairo_device_get_user_data() -> *mut c_void; + pub fn cairo_device_set_user_data(device: *mut cairo_device_t, key: *const cairo_user_data_key_t, user_data: *mut c_void, destroy: cairo_destroy_func_t) -> cairo_status_t; + pub fn cairo_device_get_user_data(device: *mut cairo_device_t, key: *const cairo_user_data_key_t) -> *mut c_void; pub fn cairo_device_acquire(device: *mut cairo_device_t) -> cairo_status_t; pub fn cairo_device_release(device: *mut cairo_device_t); pub fn cairo_device_observer_elapsed(device: *mut cairo_device_t) -> c_double; diff --git a/src/device.rs b/src/device.rs index 9f475dd4..65f50a2f 100644 --- a/src/device.rs +++ b/src/device.rs @@ -274,6 +274,12 @@ impl Device { } } } + + user_data_methods! { + Device::to_raw_none, + ffi::cairo_device_get_user_data, + ffi::cairo_device_set_user_data, + } } #[cfg(feature = "use_glib")] diff --git a/src/font/font_face.rs b/src/font/font_face.rs index d18cc0e4..e0a55bcd 100644 --- a/src/font/font_face.rs +++ b/src/font/font_face.rs @@ -126,6 +126,12 @@ impl FontFace { ffi::cairo_ft_font_face_unset_synthesize(self.to_raw_none(), synth_flags.into()) } } + + user_data_methods! { + FontFace::to_raw_none, + ffi::cairo_font_face_get_user_data, + ffi::cairo_font_face_set_user_data, + } } #[cfg(not(feature = "use_glib"))] diff --git a/src/font/scaled_font.rs b/src/font/scaled_font.rs index 941de20e..6896c84f 100644 --- a/src/font/scaled_font.rs +++ b/src/font/scaled_font.rs @@ -249,6 +249,12 @@ impl ScaledFont { matrix } + + user_data_methods! { + ScaledFont::to_raw_none, + ffi::cairo_scaled_font_get_user_data, + ffi::cairo_scaled_font_set_user_data, + } } #[cfg(not(feature = "use_glib"))] diff --git a/src/lib.rs b/src/lib.rs index f84111b3..586568b0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -63,6 +63,8 @@ macro_rules! gvalue_impl { } } +pub use user_data::UserDataKey; + pub use context::{ Context, RectangleList, @@ -150,6 +152,7 @@ pub use xcb::{ pub mod prelude; +#[macro_use] mod user_data; mod constants; pub use constants::*; mod utils; diff --git a/src/patterns.rs b/src/patterns.rs index b3f4d192..38522099 100644 --- a/src/patterns.rs +++ b/src/patterns.rs @@ -35,6 +35,14 @@ pub enum Pattern { Mesh(Mesh), } +impl Pattern { + user_data_methods! { + Pattern::as_ptr, + ffi::cairo_pattern_get_user_data, + ffi::cairo_pattern_set_user_data, + } +} + impl PatternTrait for Pattern { type PatternType = Pattern; @@ -170,6 +178,14 @@ macro_rules! pattern_type( } } + impl $pattern_type { + user_data_methods! { + $pattern_type::as_ptr, + ffi::cairo_pattern_get_user_data, + ffi::cairo_pattern_set_user_data, + } + } + impl Clone for $pattern_type { fn clone(&self) -> Self { $pattern_type { diff --git a/src/surface.rs b/src/surface.rs index b9d9d28f..2cd04625 100644 --- a/src/surface.rs +++ b/src/surface.rs @@ -196,6 +196,12 @@ impl Surface { }) } } + + user_data_methods! { + Surface::to_raw_none, + ffi::cairo_surface_get_user_data, + ffi::cairo_surface_set_user_data, + } } #[cfg(feature = "use_glib")] diff --git a/src/user_data.rs b/src/user_data.rs new file mode 100644 index 00000000..efe25af9 --- /dev/null +++ b/src/user_data.rs @@ -0,0 +1,103 @@ +use std::marker::PhantomData; + +use ffi::cairo_user_data_key_t; + +pub struct UserDataKey { + pub(crate) ffi: cairo_user_data_key_t, + marker: PhantomData<*const T>, +} + +unsafe impl Sync for UserDataKey {} + +impl UserDataKey { + pub const fn new() -> Self { + UserDataKey { + ffi: cairo_user_data_key_t { unused: 0 }, + marker: PhantomData, + } + } +} + +// In a safe API for user data we can’t make `get_user_data` +// transfer full ownership of the value to the caller (e.g. by returning `Box`) +// because `self` still has a pointer to that value +// and `get_user_data` could be called again with the same key. +// +// We also can’t return a `&T` reference that borrows from `self` +// because the value could be removed with `remove_user_data` or replaced with `set_user_data` +// while the borrow still needs to be valid. +// (Borrowing with `&mut self` would not help as `Self` can be itself reference-counted.) +// +// Therefore the value must be reference-counted. +// +// We use `Rc` over `Arc` because the types implementing these methods are `!Send` and `!Sync`. +// See + +macro_rules! user_data_methods { + ($as_ptr: expr, $ffi_get_user_data: path, $ffi_set_user_data: path,) => { + pub fn set_user_data(&self, key: &'static crate::UserDataKey, + value: std::rc::Rc) + { + unsafe extern "C" fn destructor(ptr: *mut libc::c_void) { + let ptr: *const T = ptr as _; + drop(std::rc::Rc::from_raw(ptr)) + } + // Safety: + // + // The destructor’s cast and `from_raw` are symetric + // with the `into_raw` and cast below. + // They both transfer ownership of one strong reference: + // neither of them touches the reference count. + let ptr: *const T = std::rc::Rc::into_raw(value); + let ptr = ptr as *mut T as *mut libc::c_void; + let result = unsafe { + $ffi_set_user_data($as_ptr(self), &key.ffi, ptr, Some(destructor::)) + }; + Status::from(result).ensure_valid() + } + + pub fn get_user_data(&self, key: &'static crate::UserDataKey) + -> Option> + { + // Safety: + // + // `Rc::from_raw` would normally take ownership of a strong reference for this pointer. + // But `self` still has a copy of that pointer and `get_user_data` can be called again + // with the same key. + // We use `ManuallyDrop` to avoid running the destructor of that first `Rc`, + // and return a cloned one (which increments the reference count). + // + // If `ffi_get_user_data` returns a non-null pointer, + // there was a previous call to `ffi_set_user_data` with a key with the same address. + // Either: + // + // * This was a call to a Rust `Self::set_user_data` method. + // Because that method takes a `&'static` reference, + // the key used then must live at that address until the end of the process. + // Because `UserDataKey` has a non-zero size regardless of `T`, + // no other `UserDataKey` value can have the same address. + // Therefore the `T` type was the same then at it is now and `cast` is type-safe. + // + // * Or, it is technically possible that the `set` call was to the C function directly, + // with a `cairo_user_data_key_t` in heap-allocated memory that was then freed, + // then `Box::new(UserDataKey::new()).leak()` was used to create a `&'static` + // that happens to have the same address because the allocator for `Box` + // reused that memory region. + // Since this involves a C (or FFI) call *and* is so far out of “typical” use + // of the user data functionality, we consider this a misuse of an unsafe API. + unsafe { + let ptr = $ffi_get_user_data($as_ptr(self), &key.ffi); + let ptr: *mut T = std::ptr::NonNull::new(ptr)?.cast().as_ptr(); + let rc = std::mem::ManuallyDrop::new(std::rc::Rc::from_raw(ptr)); + Some(std::rc::Rc::clone(&rc)) + } + } + + pub fn remove_user_data(&self, key: &'static crate::UserDataKey) { + let result = unsafe { + $ffi_set_user_data($as_ptr(self), &key.ffi, std::ptr::null_mut(), None) + }; + Status::from(result).ensure_valid() + } + }; +} From 9e8dc2e2f8f1d7109a3a2eb6cd1d3a1a5dd0a953 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 12 May 2019 19:04:35 +0200 Subject: [PATCH 2/3] Use the new user data API in ImageSurface::create_for_data --- src/image_surface.rs | 26 ++++++++++++++------------ src/surface.rs | 30 +++++------------------------- 2 files changed, 19 insertions(+), 37 deletions(-) diff --git a/src/image_surface.rs b/src/image_surface.rs index 75aca34e..a7cb9de4 100644 --- a/src/image_surface.rs +++ b/src/image_surface.rs @@ -3,6 +3,7 @@ // Licensed under the MIT license, see the LICENSE file or use std::ops::{Deref, DerefMut}; +use std::rc::Rc; use std::slice; #[cfg(feature = "use_glib")] @@ -14,7 +15,7 @@ use ::enums::{ }; use BorrowError; -use surface::{Surface, SurfaceExt, SurfacePriv}; +use surface::{Surface, SurfaceExt}; use Status; use std::fmt; @@ -48,23 +49,26 @@ impl ImageSurface { pub fn create_for_data + 'static>(data: D, format: Format, width: i32, height: i32, stride: i32) -> Result { - let mut data: Box + 'static> = Box::new(data); + let mut data: Box> = Box::new(data); let (ptr, len) = { - let mut data = (*data).as_mut(); + let mut data: &mut [u8] = (*data).as_mut(); (data.as_mut_ptr(), data.len()) }; assert!(len >= (height * stride) as usize); - unsafe { - let r = ImageSurface::from_raw_full( - ffi::cairo_image_surface_create_for_data(ptr, format.into(), width, height, stride)); - match r { - Ok(surface) => surface.set_user_data(&IMAGE_SURFACE_DATA, Box::new(data)).map (|_| surface), - Err(status) => Err(status) - } + let result = unsafe { + ImageSurface::from_raw_full( + ffi::cairo_image_surface_create_for_data(ptr, format.into(), width, height, stride) + ) + }; + if let Ok(surface) = &result { + static IMAGE_SURFACE_DATA: crate::UserDataKey>> = + crate::UserDataKey::new(); + surface.set_user_data(&IMAGE_SURFACE_DATA, Rc::new(data)) } + result } pub fn get_data(&mut self) -> Result { @@ -103,8 +107,6 @@ impl ImageSurface { } } -static IMAGE_SURFACE_DATA: () = (); - #[cfg(feature = "use_glib")] impl<'a> ToGlibPtr<'a, *mut ffi::cairo_surface_t> for ImageSurface { type Storage = &'a Surface; diff --git a/src/surface.rs b/src/surface.rs index 2cd04625..912c6ed3 100644 --- a/src/surface.rs +++ b/src/surface.rs @@ -2,7 +2,6 @@ // See the COPYRIGHT file at the top-level directory of this distribution. // Licensed under the MIT license, see the LICENSE file or -use std::mem; use std::ptr; use std::slice; use libc::{c_ulong, c_void}; @@ -113,6 +112,11 @@ impl Surface { let user_data = Box::into_raw(b); + unsafe extern "C" fn unbox(data: *mut c_void) { + let data: Box = Box::from_raw(data as *mut T); + drop(data); + } + let status = unsafe { let mime_type = CString::new(mime_type).unwrap(); ffi::cairo_surface_set_mime_data(self.to_raw_none(), @@ -329,30 +333,6 @@ impl fmt::Display for MappedImageSurface { } } -pub(crate) trait SurfacePriv { - unsafe fn set_user_data(&self, key: &K, data: Box) -> Result<(), Status>; -} - -impl> SurfacePriv for O { - unsafe fn set_user_data(&self, key: &K, data: Box) -> Result<(), Status> { - let ptr: *mut T = Box::into_raw(data); - - assert_eq!(mem::size_of::<*mut c_void>(), mem::size_of_val(&ptr)); - - let status = ffi::cairo_surface_set_user_data(self.as_ref().0, key as *const _ as *mut _, - ptr as *mut c_void, Some(unbox::)); - match Status::from(status) { - Status::Success => Ok(()), - x => Err(x), - } - } -} - -unsafe extern "C" fn unbox(data: *mut c_void) { - let data: Box = Box::from_raw(data as *mut T); - drop(data); -} - #[cfg(test)] mod tests { use Format; From c2528320cff57890976a80282689445327c3aed9 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 12 May 2019 20:55:32 +0200 Subject: [PATCH 3/3] Document user data API --- src/user_data.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/user_data.rs b/src/user_data.rs index efe25af9..584d630f 100644 --- a/src/user_data.rs +++ b/src/user_data.rs @@ -2,6 +2,20 @@ use std::marker::PhantomData; use ffi::cairo_user_data_key_t; +/// A key for indexing user data in various cairo types. +/// +/// Some types like [`Surface`] have `get_user_data`, `set_user_data`, and `remove_user_data` +/// methods that take `&'static UserDataKey`, where the address of that reference is significant. +/// +/// To reliably have a stable address, the expected usage is to define a `static` item: +/// +/// ``` +/// use cairo::UserDataKey; +/// static FOO: UserDataKey = UserDataKey::new(); +/// +/// # fn foo(surface: &cairo::Surface) { +/// surface.get_user_data(&FOO) +/// # ; } pub struct UserDataKey { pub(crate) ffi: cairo_user_data_key_t, marker: PhantomData<*const T>, @@ -35,6 +49,7 @@ impl UserDataKey { macro_rules! user_data_methods { ($as_ptr: expr, $ffi_get_user_data: path, $ffi_set_user_data: path,) => { + /// Attach user data to `self` for the given `key`. pub fn set_user_data(&self, key: &'static crate::UserDataKey, value: std::rc::Rc) { @@ -56,6 +71,7 @@ macro_rules! user_data_methods { Status::from(result).ensure_valid() } + /// Return the user data previously attached to `self` with the given `key`, if any. pub fn get_user_data(&self, key: &'static crate::UserDataKey) -> Option> { @@ -93,6 +109,8 @@ macro_rules! user_data_methods { } } + /// Unattach from `self` the user data associated with `key`, if any. + /// If there is no other `Rc` strong reference, the data is destroyed. pub fn remove_user_data(&self, key: &'static crate::UserDataKey) { let result = unsafe { $ffi_set_user_data($as_ptr(self), &key.ffi, std::ptr::null_mut(), None)