From 093f70a00722ff4ae4d2f9a6311fae482e64fbe2 Mon Sep 17 00:00:00 2001 From: Harald Reingruber Date: Wed, 8 Jul 2026 10:34:14 +0200 Subject: [PATCH] Fix soundness issue: avoid UB in swap_columns/swap_elements when indices alias When swap_columns is called with a == b, the previous implementation created two mutable references (&mut self[a] and &mut self[b]) to the same memory location (aliased &mut UB). Additionally, even for distinct indices a != b, using ptr::swap(&mut self[a], &mut self[b]) is UB under Miri (Stacked Borrows): the second IndexMut call creates a new unique reborrow of all of self, invalidating the tag from the first call. The same class of bug existed in: - Array::swap_elements (structure.rs) - Matrix{2,3,4}::swap_columns - Matrix{2,3,4}::swap_elements Fix: since S: BaseFloat implies S: Copy, column vectors and scalar elements are Copy. We can swap entirely without unsafe by using a temporary variable: let tmp = self[a]; self[a] = self[b]; self[b] = tmp; This avoids creating multiple mutable references simultaneously and requires no unsafe code at all. Verified clean under Miri. Tests added in tests/matrix.rs covering all three matrix sizes for: - swap_columns with equal indices (no-op, was UB) - swap_columns with distinct indices (correct swap) - swap_elements with same position (no-op, was UB) - swap_elements with same column, different rows Note: the tests verify correctness but do not catch the soundness bug in a regular cargo test run. Use cargo +nightly miri test to detect aliasing UB. Fixes: https://github.com/rustgd/cgmath/issues/565 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/matrix.rs | 48 +++++++++++++++--- src/structure.rs | 11 ++-- tests/matrix.rs | 128 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 10 deletions(-) diff --git a/src/matrix.rs b/src/matrix.rs index 814f9e42..3008cbd3 100644 --- a/src/matrix.rs +++ b/src/matrix.rs @@ -580,14 +580,26 @@ impl Matrix for Matrix2 { #[inline] fn swap_columns(&mut self, a: usize, b: usize) { - unsafe { ptr::swap(&mut self[a], &mut self[b]) }; + if a == b { + return; + } + // S: BaseFloat implies S: Copy, so column vectors are Copy — no unsafe needed. + let tmp = self[a]; + self[a] = self[b]; + self[b] = tmp; } #[inline] fn swap_elements(&mut self, a: (usize, usize), b: (usize, usize)) { + if a == b { + return; + } let (ac, ar) = a; let (bc, br) = b; - unsafe { ptr::swap(&mut self[ac][ar], &mut self[bc][br]) }; + // S: BaseFloat implies S: Copy — swap by value without unsafe. + let tmp = self[ac][ar]; + self[ac][ar] = self[bc][br]; + self[bc][br] = tmp; } fn transpose(&self) -> Matrix2 { @@ -679,14 +691,26 @@ impl Matrix for Matrix3 { #[inline] fn swap_columns(&mut self, a: usize, b: usize) { - unsafe { ptr::swap(&mut self[a], &mut self[b]) }; + if a == b { + return; + } + // S: BaseFloat implies S: Copy, so column vectors are Copy — no unsafe needed. + let tmp = self[a]; + self[a] = self[b]; + self[b] = tmp; } #[inline] fn swap_elements(&mut self, a: (usize, usize), b: (usize, usize)) { + if a == b { + return; + } let (ac, ar) = a; let (bc, br) = b; - unsafe { ptr::swap(&mut self[ac][ar], &mut self[bc][br]) }; + // S: BaseFloat implies S: Copy — swap by value without unsafe. + let tmp = self[ac][ar]; + self[ac][ar] = self[bc][br]; + self[bc][br] = tmp; } fn transpose(&self) -> Matrix3 { @@ -795,14 +819,26 @@ impl Matrix for Matrix4 { #[inline] fn swap_columns(&mut self, a: usize, b: usize) { - unsafe { ptr::swap(&mut self[a], &mut self[b]) }; + if a == b { + return; + } + // S: BaseFloat implies S: Copy, so column vectors are Copy — no unsafe needed. + let tmp = self[a]; + self[a] = self[b]; + self[b] = tmp; } #[inline] fn swap_elements(&mut self, a: (usize, usize), b: (usize, usize)) { + if a == b { + return; + } let (ac, ar) = a; let (bc, br) = b; - unsafe { ptr::swap(&mut self[ac][ar], &mut self[bc][br]) }; + // S: BaseFloat implies S: Copy — swap by value without unsafe. + let tmp = self[ac][ar]; + self[ac][ar] = self[bc][br]; + self[bc][br] = tmp; } fn transpose(&self) -> Matrix4 { diff --git a/src/structure.rs b/src/structure.rs index 89dcbfd9..eafdba5e 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -72,10 +72,13 @@ where /// Swap the elements at indices `i` and `j` in-place. #[inline] fn swap_elements(&mut self, i: usize, j: usize) { - use std::ptr; - - // Yeah, ok borrow checker – I know what I'm doing here - unsafe { ptr::swap(&mut self[i], &mut self[j]) }; + if i == j { + return; + } + // Element: Copy is a supertrait bound, so we can swap without unsafe. + let tmp = self[i]; + self[i] = self[j]; + self[j] = tmp; } /// The sum of the elements of the array. diff --git a/tests/matrix.rs b/tests/matrix.rs index 20c78c6d..d53ccbf4 100644 --- a/tests/matrix.rs +++ b/tests/matrix.rs @@ -16,6 +16,134 @@ extern crate approx; extern crate cgmath; +/// Tests for swap_columns, swap_rows, and swap_elements correctness and soundness +/// (see https://github.com/rustgd/cgmath/issues/565). +pub mod swap { + use cgmath::*; + + #[test] + fn matrix2_swap_columns_same_index() { + let original = Matrix2::new(1.0f64, 2.0, 3.0, 4.0); + let mut m = original; + m.swap_columns(0, 0); + assert_eq!(m, original); + m.swap_columns(1, 1); + assert_eq!(m, original); + } + + #[test] + fn matrix2_swap_columns_different_indices() { + let mut m = Matrix2::new(1.0f64, 2.0, 3.0, 4.0); + m.swap_columns(0, 1); + assert_eq!(m, Matrix2::new(3.0, 4.0, 1.0, 2.0)); + // swapping back gives original + m.swap_columns(1, 0); + assert_eq!(m, Matrix2::new(1.0, 2.0, 3.0, 4.0)); + } + + #[test] + fn matrix2_swap_elements_same_position() { + let original = Matrix2::new(1.0f64, 2.0, 3.0, 4.0); + let mut m = original; + m.swap_elements((0, 0), (0, 0)); + assert_eq!(m, original); + m.swap_elements((1, 1), (1, 1)); + assert_eq!(m, original); + } + + #[test] + fn matrix2_swap_elements_same_column_different_row() { + // column 0 has elements [1.0 (row0), 2.0 (row1)] + let mut m = Matrix2::new(1.0f64, 2.0, 3.0, 4.0); + m.swap_elements((0, 0), (0, 1)); + assert_eq!(m, Matrix2::new(2.0, 1.0, 3.0, 4.0)); + } + + #[test] + fn matrix3_swap_columns_same_index() { + let original = Matrix3::new(1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0); + let mut m = original; + m.swap_columns(0, 0); + assert_eq!(m, original); + m.swap_columns(1, 1); + assert_eq!(m, original); + m.swap_columns(2, 2); + assert_eq!(m, original); + } + + #[test] + fn matrix3_swap_columns_different_indices() { + let mut m = Matrix3::new(1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0); + let col0_before = m[0]; + let col2_before = m[2]; + m.swap_columns(0, 2); + assert_eq!(m[0], col2_before); + assert_eq!(m[2], col0_before); + } + + #[test] + fn matrix3_swap_elements_same_position() { + let original = Matrix3::new(1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0); + let mut m = original; + m.swap_elements((1, 1), (1, 1)); + assert_eq!(m, original); + } + + #[test] + fn matrix3_swap_elements_same_column_different_row() { + let mut m = Matrix3::new(1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0); + m.swap_elements((1, 0), (1, 2)); + // column 1 had [4, 5, 6]; after swap row0 and row2: [6, 5, 4] + assert_eq!(m[1], Vector3::new(6.0, 5.0, 4.0)); + } + + #[test] + fn matrix4_swap_columns_same_index() { + let original = Matrix4::new( + 1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, + 16.0, + ); + let mut m = original; + m.swap_columns(2, 2); + assert_eq!(m, original); + } + + #[test] + fn matrix4_swap_columns_different_indices() { + let mut m = Matrix4::new( + 1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, + 16.0, + ); + let col1_before = m[1]; + let col3_before = m[3]; + m.swap_columns(1, 3); + assert_eq!(m[1], col3_before); + assert_eq!(m[3], col1_before); + } + + #[test] + fn matrix4_swap_elements_same_position() { + let original = Matrix4::new( + 1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, + 16.0, + ); + let mut m = original; + m.swap_elements((2, 2), (2, 2)); + assert_eq!(m, original); + } + + #[test] + fn matrix4_swap_elements_same_column_different_row() { + let mut m = Matrix4::new( + 1.0f64, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, + 16.0, + ); + // column 2 has rows [9, 10, 11, 12]; swap row 0 and row 3 + m.swap_elements((2, 0), (2, 3)); + assert_eq!(m[2], Vector4::new(12.0, 10.0, 11.0, 9.0)); + } +} + pub mod matrix2 { use std::f64;