Fix soundness issues (#74)

The first commit adds tests that demonstrate undefined behavior in safe
code under `miri`.

The second commit fixes the issues with minimal API changes. The API
could be adjusted to make such issues less likely (e.g. pre-validated
fields should not be public and mutable), but this PR only provides a
semver-compatible fix.

The third commit updates documentation to more clearly communicate
alignment requirements to API users.

### Impact analysis

The alignment issue is mostly theoretical and should not impact
real-world uses.

The missing field validation results in buffer overflow, but you need to
edit the fields after the initial validation pass; I don't expect this
to be common in the wild (albeit I haven't verified that), so I don't
think it warrants a full-on security advisory, but you might want to get
an unsoundness advisory into
[RustSec](https://github.com/rustsec/advisory-db/) with the
`informational = unsound` field.

Author: Shnatsel

README.md

@@ -14,7 +14,8 @@ A Rust library providing efficient data structures and utilities for handling YU
 - **Flexible plane structure**: Efficient memory layout with configurable padding for SIMD operations
 - **Multiple chroma formats**: Support for YUV 4:2:0, 4:2:2, 4:4:4, and monochrome
 - **Builder pattern API**: Safe and ergonomic frame construction with compile-time guarantees
-- **SIMD-friendly alignment**: 64-byte alignment (8-byte on WASM) for optimal performance
+- **SIMD-friendly alignment**: non-empty planes are aligned to at least 64 bytes
+  on most targets, 8 bytes on non-WASI `wasm32`, or `align_of::<T>()` if larger
 - **WebAssembly support**: Works in both browser (`wasm32-unknown-unknown`) and WASI environments
 - **Zero-copy iterators**: Efficient row-based and pixel-based iteration without allocations
 
@@ -158,7 +159,10 @@ cargo build --target wasm32-wasi
 wasm-pack test --headless --chrome --firefox
 ```
 
-The crate automatically adjusts memory alignment for WASM targets (8-byte vs 64-byte on native).
+The crate automatically adjusts memory alignment for WebAssembly: non-WASI
+`wasm32` targets use an 8-byte minimum alignment, while WASI and native targets
+use a 64-byte minimum. Over-aligned pixel data uses `align_of::<T>()` if that is
+larger.
 
 ## Feature Flags
 

src/plane.rs

@@ -16,10 +16,19 @@
 //! # Memory Layout
 //!
 //! Planes store data in a contiguous, aligned buffer with support for padding on all sides:
-//! - Data is aligned to 64 bytes on non-WASM platforms (SIMD-friendly)
-//! - Data is aligned to 8 bytes on WASM platforms
+//! - Non-empty allocations are aligned to at least 64 bytes on most targets
+//!   (SIMD-friendly)
+//! - Non-empty allocations are aligned to at least 8 bytes on `wasm32` targets
+//!   that are not WASI
+//! - If `T` has stricter alignment requirements, the allocation uses
+//!   `std::mem::align_of::<T>()` instead
 //! - Padding pixels surround the visible area for codec algorithms that need border access
 //!
+//! More precisely, non-empty plane data is allocated with
+//! `max(DATA_ALIGNMENT, std::mem::align_of::<T>())`, where `DATA_ALIGNMENT` is
+//! 64 except on non-WASI `wasm32` targets, where it is 8. Empty planes do not
+//! allocate and must not be assumed to have this extra SIMD alignment.
+//!
 //! # API Design
 //!
 //! The public API exposes only the "visible" pixels by default, abstracting away the padding.
@@ -58,8 +67,17 @@ use crate::pixel::Pixel;
 /// # Memory Layout
 ///
 /// The data is stored in a contiguous, aligned buffer:
-/// - 64-byte alignment on non-WASM platforms (optimized for SIMD operations)
-/// - 8-byte alignment on WASM platforms
+/// - Non-empty allocations are aligned to at least 64 bytes on most targets
+///   (optimized for SIMD operations)
+/// - Non-empty allocations are aligned to at least 8 bytes on `wasm32` targets
+///   that are not WASI
+/// - If `T` has stricter alignment requirements, the allocation uses
+///   `std::mem::align_of::<T>()` instead
+///
+/// More precisely, non-empty plane data is allocated with
+/// `max(DATA_ALIGNMENT, std::mem::align_of::<T>())`, where `DATA_ALIGNMENT` is
+/// 64 except on non-WASI `wasm32` targets, where it is 8. Empty planes do not
+/// allocate and must not be assumed to have this extra SIMD alignment.
 ///
 /// The visible pixels are surrounded by optional padding pixels. The public API
 /// provides access only to the visible area by default; padding access requires
@@ -115,6 +133,18 @@ impl<T> Plane<T> {
     ///
     /// [`data_mut`][Plane::data_mut] can be used to initialize the underlying memory.
     ///
+    /// # Allocation Alignment
+    ///
+    /// For non-empty planes, the allocation returned by [`data`][Plane::data]
+    /// and [`data_mut`][Plane::data_mut] is aligned to
+    /// `max(DATA_ALIGNMENT, std::mem::align_of::<T>())`, where `DATA_ALIGNMENT`
+    /// is 64 except on non-WASI `wasm32` targets, where it is 8. This matters
+    /// if you use unsafe code to access the backing pointer directly, especially
+    /// with over-aligned `T`.
+    ///
+    /// Empty planes do not allocate and must not be assumed to have the extra
+    /// SIMD alignment beyond what Rust requires for an empty `[T]` slice.
+    ///
     /// # Example
     ///
     /// ```
@@ -138,11 +168,15 @@ impl<T> Plane<T> {
     #[inline]
     #[must_use]
     pub fn new_uninit(geometry: PlaneGeometry) -> Plane<MaybeUninit<T>> {
-        let rows = geometry.alloc_height();
-        let pixels = rows.saturating_mul(geometry.stride);
+        let geometry = geometry
+            .normalized()
+            .expect("plane geometry dimensions must not overflow");
+        let pixels = geometry
+            .allocation_len()
+            .expect("plane allocation size must not overflow usize");
 
         Plane {
-            data: AlignedData::new_uninit(pixels.get()),
+            data: AlignedData::new_uninit(pixels),
             geometry,
         }
     }
@@ -198,8 +232,12 @@ impl<T> Plane<MaybeUninit<T>> {
 impl<T: Pixel> Plane<T> {
     /// Creates a new plane with the given geometry, initialized with zero-valued pixels.
     pub(crate) fn new(geometry: PlaneGeometry) -> Self {
-        let rows = geometry.alloc_height();
-        let pixels = rows.get() * geometry.stride.get();
+        let geometry = geometry
+            .normalized()
+            .expect("plane geometry dimensions must not overflow");
+        let pixels = geometry
+            .allocation_len()
+            .expect("plane allocation size must not overflow usize");
 
         Self {
             data: AlignedData::new(pixels),

src/plane/aligned.rs

@@ -1,14 +1,15 @@
 use std::alloc::{Layout, alloc, alloc_zeroed, dealloc, handle_alloc_error};
 use std::fmt::Debug;
 use std::marker::PhantomData;
-use std::mem::{ManuallyDrop, MaybeUninit};
+use std::mem::{ManuallyDrop, MaybeUninit, align_of};
 use std::num::NonZeroUsize;
 use std::ops::{Deref, DerefMut};
 use std::ptr::NonNull;
 
 use crate::pixel::Pixel;
 
-// Minimum data alignment to help with SIMD
+// Minimum data alignment to help with SIMD. Non-empty allocations use this or
+// `align_of::<T>()`, whichever is larger.
 const DATA_ALIGNMENT: usize = {
     if cfg!(target_arch = "wasm32") && cfg!(not(target_os = "wasi")) {
         // wasm32-unknown-unknown, wasm32-unknown-emscripten
@@ -34,13 +35,18 @@ unsafe impl<T: Sync> Sync for AlignedData<T> {}
 impl<T> AlignedData<T> {
     const fn layout(len: NonZeroUsize) -> Layout {
         const { assert!(DATA_ALIGNMENT.is_power_of_two()) };
+        let alignment = if align_of::<T>() > DATA_ALIGNMENT {
+            align_of::<T>()
+        } else {
+            DATA_ALIGNMENT
+        };
         let t_size = const { NonZeroUsize::new(size_of::<T>()).expect("T is Sized") };
 
         let size = len
             .checked_mul(t_size)
             .expect("allocation size does not overflow usize");
 
-        match Layout::from_size_align(size.get(), DATA_ALIGNMENT) {
+        match Layout::from_size_align(size.get(), alignment) {
             Ok(l) => l,
             _ => panic!("invalid layout"),
         }
@@ -219,6 +225,25 @@ mod tests {
         AlignedData::<String>::new_uninit(0);
     }
 
+    #[cfg(miri)]
+    #[test]
+    fn new_uninit_underaligns_overaligned_types() {
+        #[allow(dead_code)]
+        #[repr(align(1048576))]
+        struct OverAligned([u8; 1]);
+
+        // Issue: `AlignedData::new_uninit` allocates with `DATA_ALIGNMENT`
+        // rather than `max(DATA_ALIGNMENT, align_of::<T>())`. Safe callers can
+        // therefore create `AlignedData<MaybeUninit<T>>` for an over-aligned
+        // `T`, and the safe slice accessors form references whose required
+        // alignment is stronger than the allocation's layout. The public
+        // `Plane::new_uninit` padding API forwards to this helper for arbitrary
+        // `T`, so this can be reached without an unsafe call before any
+        // `assume_init`.
+        let mut data = AlignedData::<OverAligned>::new_uninit(1);
+        data[0].write(OverAligned([0]));
+    }
+
     #[test]
     #[should_panic(expected = "invalid layout")]
     fn invalid_layout_panic() {

src/plane/geometry.rs

@@ -92,6 +92,29 @@ impl PlaneGeometry {
         Self::new(width, height, 0, 0, 0, 0, subsampling_x, subsampling_y)
     }
 
+    #[inline]
+    pub(crate) fn normalized(self) -> Option<Self> {
+        Self::new(
+            self.width.get(),
+            self.height.get(),
+            self.pad_left,
+            self.pad_right,
+            self.pad_top,
+            self.pad_bottom,
+            self.subsampling_x.get(),
+            self.subsampling_y.get(),
+        )
+    }
+
+    #[inline]
+    pub(crate) fn allocation_len(self) -> Option<usize> {
+        self.height
+            .get()
+            .checked_add(self.pad_top)?
+            .checked_add(self.pad_bottom)?
+            .checked_mul(self.stride.get())
+    }
+
     /// Returns a new [`PlaneGeometry`] based on `self` and according to `subsampling`.
     ///
     /// Returns:

src/plane/tests.rs

@@ -59,6 +59,20 @@ fn plane_new_u16() {
     }
 }
 
+#[cfg(miri)]
+#[test]
+fn mutable_geometry_fields_can_break_row_slice_invariant() {
+    let mut geometry = simple_geometry(1, 1);
+    geometry.pad_left = 1;
+
+    // Regression test: `PlaneGeometry` fields are public, so constructors must
+    // restore the dependent `stride = width + pad_left + pad_right` invariant
+    // before row iteration relies on it.
+    let plane: Plane<u8> = Plane::new(geometry);
+    assert_eq!(plane.geometry.stride.get(), 2);
+    assert_eq!(plane.rows().next(), Some(&[0][..]));
+}
+
 #[test]
 fn plane_dimensions() {
     let geometry = simple_geometry(16, 9);