From d6a59bd4ad8b3db0b1450c8caf01c1efb003752d Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Tue, 5 May 2026 22:10:01 +0100 Subject: [PATCH 1/3] Commit initial UB PoCs --- src/plane/aligned.rs | 19 +++++++++++++++++++ src/plane/tests.rs | 18 ++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/plane/aligned.rs b/src/plane/aligned.rs index 0b14292..9812d0d 100644 --- a/src/plane/aligned.rs +++ b/src/plane/aligned.rs @@ -219,6 +219,25 @@ mod tests { AlignedData::::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::())`. Safe callers can + // therefore create `AlignedData>` 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::::new_uninit(1); + data[0].write(OverAligned([0])); + } + #[test] #[should_panic(expected = "invalid layout")] fn invalid_layout_panic() { diff --git a/src/plane/tests.rs b/src/plane/tests.rs index 2b2f167..bc1c99f 100644 --- a/src/plane/tests.rs +++ b/src/plane/tests.rs @@ -59,6 +59,24 @@ 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; + + // Issue: `Plane::rows` and `Plane::rows_mut` use `get_unchecked` under the + // invariant that `pad_left + width <= stride`. `PlaneGeometry::new` creates + // values satisfying that invariant, but the fields are public and can be + // mutated afterwards. A safe constructor that trusts such a geometry can + // then build a plane where safe row iteration creates an out-of-bounds + // slice. With the `padding_api` feature, external safe code can supply a + // mutated geometry to `Plane::new_uninit`; this crate-local constructor + // demonstrates the same trusted invariant without needing unsafe setup. + let plane: Plane = Plane::new(geometry); + let _ = plane.rows().next(); +} + #[test] fn plane_dimensions() { let geometry = simple_geometry(16, 9); From 83b57f507bcd7f6eede077fd1dbd6f4b6d3c694d Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Wed, 6 May 2026 20:41:54 +0100 Subject: [PATCH 2/3] Fix the soundness issues and convert UB proof-of-concepts into regression tests --- README.md | 8 ++++++-- src/plane.rs | 18 +++++++++++++----- src/plane/aligned.rs | 12 +++++++++--- src/plane/geometry.rs | 23 +++++++++++++++++++++++ src/plane/tests.rs | 14 +++++--------- 5 files changed, 56 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index ad23d05..32f6496 100644 --- a/README.md +++ b/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::()` 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::()` if that is +larger. ## Feature Flags diff --git a/src/plane.rs b/src/plane.rs index bf701ee..a57659c 100644 --- a/src/plane.rs +++ b/src/plane.rs @@ -138,11 +138,15 @@ impl Plane { #[inline] #[must_use] pub fn new_uninit(geometry: PlaneGeometry) -> Plane> { - 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 +202,12 @@ impl Plane> { impl Plane { /// 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), diff --git a/src/plane/aligned.rs b/src/plane/aligned.rs index 9812d0d..24628ed 100644 --- a/src/plane/aligned.rs +++ b/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::()`, 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 Sync for AlignedData {} impl AlignedData { const fn layout(len: NonZeroUsize) -> Layout { const { assert!(DATA_ALIGNMENT.is_power_of_two()) }; + let alignment = if align_of::() > DATA_ALIGNMENT { + align_of::() + } else { + DATA_ALIGNMENT + }; let t_size = const { NonZeroUsize::new(size_of::()).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"), } diff --git a/src/plane/geometry.rs b/src/plane/geometry.rs index d720364..a6a8089 100644 --- a/src/plane/geometry.rs +++ b/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::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 { + 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: diff --git a/src/plane/tests.rs b/src/plane/tests.rs index bc1c99f..65e6a06 100644 --- a/src/plane/tests.rs +++ b/src/plane/tests.rs @@ -65,16 +65,12 @@ fn mutable_geometry_fields_can_break_row_slice_invariant() { let mut geometry = simple_geometry(1, 1); geometry.pad_left = 1; - // Issue: `Plane::rows` and `Plane::rows_mut` use `get_unchecked` under the - // invariant that `pad_left + width <= stride`. `PlaneGeometry::new` creates - // values satisfying that invariant, but the fields are public and can be - // mutated afterwards. A safe constructor that trusts such a geometry can - // then build a plane where safe row iteration creates an out-of-bounds - // slice. With the `padding_api` feature, external safe code can supply a - // mutated geometry to `Plane::new_uninit`; this crate-local constructor - // demonstrates the same trusted invariant without needing unsafe setup. + // 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 = Plane::new(geometry); - let _ = plane.rows().next(); + assert_eq!(plane.geometry.stride.get(), 2); + assert_eq!(plane.rows().next(), Some(&[0][..])); } #[test] From 7b1eef68b965368e360bd4453e1ae0b8578e5ba8 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Wed, 6 May 2026 20:43:42 +0100 Subject: [PATCH 3/3] Update documentation to more clearly explain alignment invariants --- src/plane.rs | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/src/plane.rs b/src/plane.rs index a57659c..944c589 100644 --- a/src/plane.rs +++ b/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::()` 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::())`, 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::()` instead +/// +/// More precisely, non-empty plane data is allocated with +/// `max(DATA_ALIGNMENT, std::mem::align_of::())`, 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 Plane { /// /// [`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::())`, 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 /// /// ```