[librsvg: 1/3] Fix unsound transmute from `[u8]` to `[CairoARGB]`




commit e79934d0f73f11348a3b90dbcf7a491b8e7f9823
Author: Michael Howell <michael notriddle com>
Date:   Wed Mar 2 10:43:54 2022 -0700

    Fix unsound transmute from `[u8]` to `[CairoARGB]`
    
    Related to #450
    
    Part-of: <https://gitlab.gnome.org/GNOME/librsvg/-/merge_requests/671>

 src/surface_utils/mod.rs            | 20 +++++++++++++-------
 src/surface_utils/shared_surface.rs | 26 ++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 11 deletions(-)
---
diff --git a/src/surface_utils/mod.rs b/src/surface_utils/mod.rs
index 7fcd1b5e4..fed60cbc1 100644
--- a/src/surface_utils/mod.rs
+++ b/src/surface_utils/mod.rs
@@ -1,6 +1,6 @@
 //! Various utilities for working with Cairo image surfaces.
 
-use std::mem;
+use std::alloc;
 use std::slice;
 
 pub mod iterators;
@@ -29,7 +29,7 @@ pub type GdkPixbufRGB = rgb::RGB8;
 
 /// Analogous to `rgb::FromSlice`, to convert from `[T]` to `[CairoARGB]`
 #[allow(clippy::upper_case_acronyms)]
-pub trait AsCairoARGB<T: Copy> {
+pub trait AsCairoARGB {
     /// Reinterpret slice as `CairoARGB` pixels.
     fn as_cairo_argb(&self) -> &[CairoARGB];
 
@@ -37,15 +37,21 @@ pub trait AsCairoARGB<T: Copy> {
     fn as_cairo_argb_mut(&mut self) -> &mut [CairoARGB];
 }
 
-impl<T: Copy> AsCairoARGB<T> for [T] {
+// SAFETY: transmuting from u32 to CairoRGB is based on the following assumptions:
+//  * there are no invalid bit representations for ARGB
+//  * u32 and ARGB are the same size
+//  * u32 is sufficiently aligned
+impl AsCairoARGB for [u32] {
     fn as_cairo_argb(&self) -> &[CairoARGB] {
-        debug_assert_eq!(4, mem::size_of::<CairoARGB>() / mem::size_of::<T>());
-        unsafe { slice::from_raw_parts(self.as_ptr() as *const _, self.len() / 4) }
+        const LAYOUT_U32: alloc::Layout = alloc::Layout::new::<u32>();
+        const LAYOUT_ARGB: alloc::Layout = alloc::Layout::new::<CairoARGB>();
+        let _: [(); LAYOUT_U32.size()] = [(); LAYOUT_ARGB.size()];
+        let _: [(); 0] = [(); LAYOUT_U32.align() % LAYOUT_ARGB.align()];
+        unsafe { slice::from_raw_parts(self.as_ptr() as *const _, self.len()) }
     }
 
     fn as_cairo_argb_mut(&mut self) -> &mut [CairoARGB] {
-        debug_assert_eq!(4, mem::size_of::<CairoARGB>() / mem::size_of::<T>());
-        unsafe { slice::from_raw_parts_mut(self.as_ptr() as *mut _, self.len() / 4) }
+        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut _, self.len()) }
     }
 }
 
diff --git a/src/surface_utils/shared_surface.rs b/src/surface_utils/shared_surface.rs
index adbf8e1fe..c0ec73dec 100644
--- a/src/surface_utils/shared_surface.rs
+++ b/src/surface_utils/shared_surface.rs
@@ -1205,13 +1205,22 @@ impl<'a> Iterator for Rows<'a> {
 
         self.next_row += 1;
 
+        // SAFETY: this code assumes that cairo image surface data is correctly
+        // aligned for u32. This assumption is justified by the Cairo docs,
+        // which say this:
+        //
+        // https://cairographics.org/manual/cairo-Image-Surfaces.html#cairo-image-surface-create-for-data
+        //
+        // > This pointer must be suitably aligned for any kind of variable,
+        // > (for example, a pointer returned by malloc).
         unsafe {
-            let row_ptr = self
+            let row_ptr: *const u8 = self
                 .surface
                 .data_ptr
                 .as_ptr()
                 .offset(row as isize * self.surface.stride);
-            let row_of_bytes = slice::from_raw_parts(row_ptr, self.surface.width as usize * 4);
+            let row_of_bytes: &[u32] =
+                slice::from_raw_parts(row_ptr as *const u32, self.surface.width as usize);
             let pixels = row_of_bytes.as_cairo_argb();
             assert!(pixels.len() == self.surface.width as usize);
             Some(pixels)
@@ -1231,14 +1240,23 @@ impl<'a> Iterator for RowsMut<'a> {
 
         self.next_row += 1;
 
+        // SAFETY: this code assumes that cairo image surface data is correctly
+        // aligned for u32. This assumption is justified by the Cairo docs,
+        // which say this:
+        //
+        // https://cairographics.org/manual/cairo-Image-Surfaces.html#cairo-image-surface-create-for-data
+        //
+        // > This pointer must be suitably aligned for any kind of variable,
+        // > (for example, a pointer returned by malloc).
         unsafe {
             // We do this with raw pointers, instead of re-slicing the &mut self.data[....],
             // because with the latter we can't synthesize an appropriate lifetime for
             // the return value.
 
             let data_ptr = self.data.as_mut_ptr();
-            let row_ptr = data_ptr.offset(row as isize * self.stride as isize);
-            let row_of_bytes = slice::from_raw_parts_mut(row_ptr, self.width as usize * 4);
+            let row_ptr: *mut u8 = data_ptr.offset(row as isize * self.stride as isize);
+            let row_of_bytes: &mut [u32] =
+                slice::from_raw_parts_mut(row_ptr as *mut u32, self.width as usize);
             let pixels = row_of_bytes.as_cairo_argb_mut();
             assert!(pixels.len() == self.width as usize);
             Some(pixels)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]