Alignment fixes (soundness) (#80)

* Alignment fixes (soundness)

* cargo fmt

* Add codegen-check, verify

* Disable coverage for codegen-check

* Missing cfg(feature(coverage_attribute))

Author: simonask

CHANGELOG.md

@@ -5,6 +5,24 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Fixes
+
+- Fixed a soundness hole in `Vector4` when using smaller scalar types.
+
+### Breaking changes
+
+- Vector types and references can no longer be converted to and from `glam`
+  types by reference. This fixes a soundness hole, because it relaxes the
+  alignment requirement of `Vector4`, which would otherwise result in a size
+  mismatch between `Vector4` and `glam` vectors for smaller scalar types. I.e.
+  `glam::U8Vec4` has alignment 1. In general, it is brittle to rely on the
+  specific alignment of `glam` vector types, because they are highly
+  architecture dependent, and it is unlikely that it gains anything, because
+  unaligned vector register loads are no longer slow (with AVX, the `vaddps`
+  instruction supports unaligned loads natively).
+
 ## [0.17.0] - 2025-05-03
 
 ### Added

Cargo.toml

@@ -1,5 +1,5 @@
 [workspace]
-members = ["tests/wasmtime-guest"]
+members = [ "codegen-check","tests/wasmtime-guest"]
 
 [package]
 name = "glamour"
@@ -28,7 +28,7 @@ glam = { version = "0.30.0", default-features = false, features = [
     "bytemuck",
     "approx",
 ] }
-bytemuck = { version = "^1.8", default-features = false }
+bytemuck = { version = "^1.23", default-features = false, features = ["must_cast"] }
 num-traits = { version = "^0.2.19", default-features = false }
 approx = "^0.5"
 facet = { version = "0.18.4", optional = true, default-features = false }

README.md

@@ -49,7 +49,7 @@ let size: Size2<MyUnit> = Size2 { width: 100.0, height: 200.0 };
 let vector_untyped: &Vector4<f32> = vector.as_untyped();
 
 // Use glam when needed:
-let vector_raw: &glam::Vec4 = glamour::Transparent::peel_ref(&vector);
+let vector_raw: glam::Vec4 = vector.to_raw();
 ```
 
 [See the documentation module for more examples.](crate::docs::examples)

codegen-check/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+name = "codegen-check"
+version = "0.1.0"
+edition = "2024"
+publish = false
+
+[lib]
+path = "lib.rs"
+
+[dependencies]
+glamour.path = ".."
+
+[lints.rust]
+unexpected_cfgs = { level = "warn", check-cfg = [
+    'cfg(coverage,coverage_nightly)',
+] }

codegen-check/lib.rs

@@ -0,0 +1,28 @@
+//! Codegen checks for the `glamour` crate.
+//!
+//! This is intended to be run manually when checking that glamour types
+//! generate specific assembly output.
+//!
+//! 1. Install `cargo-show-asm` (`cargo install cargo-show-asm`).
+//! 2. Run `cargo show-asm --release --target-cpu=native --target codegen-check
+//!    <symbol>`, where `<symbol>` is one of the functions below.
+//!
+//! Note that the `#[inline(never)]` is required to force the compiler to
+//! generate the symbol.
+
+#![cfg_attr(coverage, feature(coverage_attribute))]
+#![cfg_attr(coverage, coverage(off))]
+
+use glamour::Vector4;
+
+#[inline(never)]
+pub fn sum_f32x4(v: &[Vector4<f32>]) -> Vector4<f32> {
+    // This should generate a tight SIMD loop using unaligned loads or `vaddps`
+    // when the CPU supports it.
+    v.iter().copied().sum()
+}
+
+#[inline(never)]
+pub fn sum_u8x4(v: &[Vector4<u8>]) -> Vector4<u8> {
+    v.iter().copied().sum()
+}

src/angle.rs

@@ -31,7 +31,7 @@ pub struct Angle<U: Scalar = f32> {
 }
 unsafe impl<T: Scalar> Zeroable for Angle<T> {}
 unsafe impl<T: Scalar> Pod for Angle<T> {}
-unsafe impl<T: Scalar> Transparent for Angle<T> {
+impl<T: Scalar> Transparent for Angle<T> {
     type Wrapped = T;
 }
 
@@ -432,8 +432,6 @@ mod tests {
         assert_ulps_eq, assert_ulps_ne,
     };
 
-    use crate::{peel_mut, peel_ref};
-
     use super::*;
 
     type Angle = super::Angle<f32>;
@@ -517,9 +515,7 @@ mod tests {
 
     #[test]
     fn angle_cast() {
-        let mut a = Angle::CIRCLE;
-        let _: &f32 = peel_ref(&a);
-        let _: &mut f32 = peel_mut(&mut a);
+        let a = Angle::CIRCLE;
         let _: f32 = peel(a);
         let _: f32 = a.to_radians();
         let _: Angle = 1.0.into();

src/bindings/vec.rs

@@ -26,6 +26,8 @@ pub trait Vector:
     + DivAssign<Self>
     + Rem<Self, Output = Self>
     + RemAssign<Self>
+    + core::iter::Sum
+    + core::iter::Product
     + for<'a> core::iter::Sum<&'a Self>
     + for<'a> core::iter::Product<&'a Self>
 {

src/forward.rs

@@ -179,7 +179,7 @@ macro_rules! forward_fn_self_ref {
         {
             crate::wrap_ret_val!(
                 $($($ret)* => )*
-                crate::peel_ref(self).$fn_name(
+                crate::peel_copy(self).$fn_name(
                     $(crate::forward_arg!($arg_name: $arg_ty)),*
                 )
             )
@@ -436,7 +436,7 @@ macro_rules! forward_arg {
         crate::peel($arg)
     };
     ($arg:ident: ref_self) => {
-        crate::peel_ref($arg)
+        &crate::peel_copy($arg)
     };
     ($arg:ident: ref_scalar_array_4) => {
         $arg

src/impl_ops.rs

@@ -252,12 +252,16 @@ macro_rules! impl_assign_ops {
         $(
             impl<T: crate::Unit> core::ops::$op_trait<$rhs_ty<T>> for $lhs_ty<T> {
                 fn $op_name(&mut self, rhs: $rhs_ty<T>) {
-                    core::ops::$op_trait::$op_name(crate::peel_mut(self), crate::peel(rhs))
+                    let mut value = crate::peel_copy(self);
+                    core::ops::$op_trait::$op_name(&mut value, crate::peel(rhs));
+                    *self = crate::wrap(value);
                 }
             }
             impl<T: crate::Unit> core::ops::$op_trait<&$rhs_ty<T>> for $lhs_ty<T> {
                 fn $op_name(&mut self, rhs: &$rhs_ty<T>) {
-                    core::ops::$op_trait::$op_name(crate::peel_mut(self), crate::peel(*rhs))
+                    let mut value = crate::peel_copy(self);
+                    core::ops::$op_trait::$op_name(&mut value, crate::peel(*rhs));
+                    *self = crate::wrap(value);
                 }
             }
         )*
@@ -275,12 +279,16 @@ macro_rules! impl_scalar_assign_ops {
         $(
             impl<T: crate::Unit<Scalar = $rhs_ty>> core::ops::$op_trait<$rhs_ty> for $lhs_ty<T> {
                 fn $op_name(&mut self, rhs: $rhs_ty) {
-                    core::ops::$op_trait::$op_name(crate::peel_mut(self), rhs);
+                    let mut value = crate::peel_copy(self);
+                    core::ops::$op_trait::$op_name(&mut value, rhs);
+                    *self = crate::wrap(value);
                 }
             }
             impl<T: crate::Unit<Scalar = $rhs_ty>> core::ops::$op_trait<&$rhs_ty> for $lhs_ty<T> {
                 fn $op_name(&mut self, rhs: &$rhs_ty) {
-                    core::ops::$op_trait::$op_name(crate::peel_mut(self), *rhs);
+                    let mut value = crate::peel_copy(self);
+                    core::ops::$op_trait::$op_name(&mut value, *rhs);
+                    *self = crate::wrap(value);
                 }
             }
         )*

src/impl_traits.rs

@@ -13,7 +13,7 @@ macro_rules! impl_basic_traits {
 
         impl<T: Unit> PartialEq for $base_type_name<T> {
             fn eq(&self, other: &Self) -> bool {
-                *crate::peel_ref(self) == *crate::peel_ref(other)
+                crate::peel_copy(self) == crate::peel_copy(other)
             }
         }
 
@@ -76,7 +76,7 @@ macro_rules! impl_basic_traits_vectorlike {
             where
                 H: core::hash::Hasher,
             {
-                core::hash::Hash::hash(crate::peel_ref(self), state)
+                core::hash::Hash::hash(&crate::peel_copy(self), state)
             }
         }
 
@@ -132,12 +132,12 @@ macro_rules! impl_basic_traits_vectorlike {
 
             #[must_use]
             fn abs_diff_eq(&self, other: &Self, epsilon: Self::Epsilon) -> bool {
-                crate::peel_ref(self).abs_diff_eq(crate::peel_ref(other), epsilon)
+                crate::peel_copy(self).abs_diff_eq(&crate::peel_copy(other), epsilon)
             }
 
             #[must_use]
             fn abs_diff_ne(&self, other: &Self, epsilon: Self::Epsilon) -> bool {
-                crate::peel_ref(self).abs_diff_ne(crate::peel_ref(other), epsilon)
+                crate::peel_copy(self).abs_diff_ne(&crate::peel_copy(other), epsilon)
             }
         }
 
@@ -154,7 +154,7 @@ macro_rules! impl_basic_traits_vectorlike {
                 epsilon: Self::Epsilon,
                 max_relative: Self::Epsilon,
             ) -> bool {
-                crate::peel_ref(self).relative_eq(crate::peel_ref(other), epsilon, max_relative)
+                crate::peel_copy(self).relative_eq(&crate::peel_copy(other), epsilon, max_relative)
             }
 
             #[must_use]
@@ -164,7 +164,7 @@ macro_rules! impl_basic_traits_vectorlike {
                 epsilon: Self::Epsilon,
                 max_relative: Self::Epsilon,
             ) -> bool {
-                crate::peel_ref(self).relative_ne(crate::peel_ref(other), epsilon, max_relative)
+                crate::peel_copy(self).relative_ne(&crate::peel_copy(other), epsilon, max_relative)
             }
         }
 
@@ -176,12 +176,12 @@ macro_rules! impl_basic_traits_vectorlike {
 
             #[must_use]
             fn ulps_eq(&self, other: &Self, epsilon: Self::Epsilon, max_ulps: u32) -> bool {
-                crate::peel_ref(self).ulps_eq(crate::peel_ref(other), epsilon, max_ulps)
+                crate::peel_copy(self).ulps_eq(&crate::peel_copy(other), epsilon, max_ulps)
             }
 
             #[must_use]
             fn ulps_ne(&self, other: &Self, epsilon: Self::Epsilon, max_ulps: u32) -> bool {
-                crate::peel_ref(self).ulps_ne(crate::peel_ref(other), epsilon, max_ulps)
+                crate::peel_copy(self).ulps_ne(&crate::peel_copy(other), epsilon, max_ulps)
             }
         }
 
@@ -283,29 +283,6 @@ macro_rules! impl_basic_traits_vectorlike {
                     crate::peel(value)
                 }
             }
-            impl<T: Unit<Scalar = $scalar>> core::borrow::Borrow<$glam_ty> for $base_type_name<T>
-            {
-                fn borrow(&self) -> &$glam_ty {
-                    crate::peel_ref(self)
-                }
-            }
-            impl<T: Unit<Scalar = $scalar>> core::borrow::BorrowMut<$glam_ty>
-                for $base_type_name<T>
-            {
-                fn borrow_mut(&mut self) -> &mut $glam_ty {
-                    crate::peel_mut(self)
-                }
-            }
-            impl<T: Unit<Scalar = $scalar>> AsRef<$glam_ty> for $base_type_name<T> {
-                fn as_ref(&self) -> &$glam_ty {
-                    crate::peel_ref(self)
-                }
-            }
-            impl<T: Unit<Scalar = $scalar>> AsMut<$glam_ty> for $base_type_name<T> {
-                fn as_mut(&mut self) -> &mut $glam_ty {
-                    crate::peel_mut(self)
-                }
-            }
         )*
     };
 }