From d609a2f174eaeea6108b5d2e0912626793305194 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Crozet?= Date: Tue, 3 Aug 2021 17:02:42 +0200 Subject: [PATCH] Address unsoundness in the resizing API. --- Cargo.toml | 1 - src/base/allocator.rs | 9 ++-- src/base/array_storage.rs | 21 +++++--- src/base/construction.rs | 16 ------ src/base/default_allocator.rs | 75 +++++++++------------------ src/base/edition.rs | 78 +++++++++++++++++++++++++++-- src/base/matrix.rs | 23 +++------ src/base/vec_storage.rs | 18 +++++-- src/third_party/mint/mint_matrix.rs | 41 +++++++++------ 9 files changed, 161 insertions(+), 121 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d10db84a..9c433b2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,6 @@ io = [ "pest", "pest_derive" ] compare = [ "matrixcompare-core" ] libm = [ "simba/libm" ] libm-force = [ "simba/libm_force" ] -no_unsound_assume_init = [ ] macros = [ "nalgebra-macros" ] # Conversion diff --git a/src/base/allocator.rs b/src/base/allocator.rs index 8ad78699..29286420 100644 --- a/src/base/allocator.rs +++ b/src/base/allocator.rs @@ -25,8 +25,6 @@ pub trait Allocator: Any + Sized { /// The type of buffer with uninitialized components this allocator can instanciate. type BufferUninit: RawStorageMut, R, C> + IsContiguous; - /// Allocates a buffer with the given number of rows and columns without initializing its content. - unsafe fn allocate_uninitialized(nrows: R, ncols: C) -> MaybeUninit; /// Allocates a buffer with the given number of rows and columns without initializing its content. fn allocate_uninit(nrows: R, ncols: C) -> Self::BufferUninit; @@ -55,10 +53,9 @@ pub trait Reallocator: /// /// # Safety /// The following invariants must be respected by the implementors of this method: - /// * The copy is performed as if both were just arrays (without a matrix structure). - /// * If `buf` is larger than the output size, then extra elements of `buf` are truncated. - /// * If `buf` is smaller than the output size, then extra elements at the end of the output - /// matrix (seen as an array) are left uninitialized. + /// * The copy is performed as if both were just arrays (without taking into account the matrix structure). + /// * If the underlying buffer is being shrunk, the removed elements must **not** be dropped + /// by this method. Dropping them is the responsibility of the caller. unsafe fn reallocate_copy( nrows: RTo, ncols: CTo, diff --git a/src/base/array_storage.rs b/src/base/array_storage.rs index 65a43c2b..5ed97f46 100644 --- a/src/base/array_storage.rs +++ b/src/base/array_storage.rs @@ -12,8 +12,6 @@ use serde::ser::SerializeSeq; use serde::{Deserialize, Deserializer, Serialize, Serializer}; #[cfg(feature = "serde-serialize-no-std")] use std::marker::PhantomData; -#[cfg(feature = "serde-serialize-no-std")] -use std::mem; #[cfg(feature = "abomonation-serialize")] use abomonation::Abomonation; @@ -24,6 +22,7 @@ use crate::base::dimension::{Const, ToTypenum}; use crate::base::storage::{IsContiguous, Owned, RawStorage, RawStorageMut, ReshapableStorage}; use crate::base::Scalar; use crate::Storage; +use std::mem::{self, MaybeUninit}; /* * @@ -158,8 +157,8 @@ where fn reshape_generic(self, _: Const, _: Const) -> Self::Output { unsafe { - let data: [[T; R2]; C2] = std::mem::transmute_copy(&self.0); - std::mem::forget(self.0); + let data: [[T; R2]; C2] = mem::transmute_copy(&self.0); + mem::forget(self.0); ArrayStorage(data) } } @@ -238,19 +237,27 @@ where where V: SeqAccess<'a>, { - let mut out: Self::Value = unsafe { mem::MaybeUninit::uninit().assume_init() }; + let mut out: ArrayStorage, R, C> = + DefaultAllocator::allocate_uninit(Const::, Const::); let mut curr = 0; while let Some(value) = visitor.next_element()? { *out.as_mut_slice() .get_mut(curr) - .ok_or_else(|| V::Error::invalid_length(curr, &self))? = value; + .ok_or_else(|| V::Error::invalid_length(curr, &self))? = MaybeUninit::new(value); curr += 1; } if curr == R * C { - Ok(out) + // Safety: all the elements have been initialized. + unsafe { Ok(, Const>>::assume_init(out)) } } else { + for i in 0..curr { + // Safety: + // - We couldn’t initialize the whole storage. Drop the ones we initialized. + unsafe { std::ptr::drop_in_place(out.as_mut_slice()[i].as_mut_ptr()) }; + } + Err(V::Error::invalid_length(curr, &self)) } } diff --git a/src/base/construction.rs b/src/base/construction.rs index 0e62c54a..2ba3c1cf 100644 --- a/src/base/construction.rs +++ b/src/base/construction.rs @@ -50,16 +50,6 @@ impl OMatrix where DefaultAllocator: Allocator, { - /// Creates a new uninitialized matrix. - /// - /// # Safety - /// If the matrix has a compile-time dimension, this panics - /// if `nrows != R::to_usize()` or `ncols != C::to_usize()`. - #[inline] - pub unsafe fn new_uninitialized_generic(nrows: R, ncols: C) -> MaybeUninit { - Self::from_uninitialized_data(DefaultAllocator::allocate_uninitialized(nrows, ncols)) - } - /// Creates a matrix with all its elements set to `elem`. #[inline] pub fn from_element_generic(nrows: R, ncols: C, elem: T) -> Self { @@ -381,12 +371,6 @@ where */ macro_rules! impl_constructors( ($($Dims: ty),*; $(=> $DimIdent: ident: $DimBound: ident),*; $($gargs: expr),*; $($args: ident),*) => { - /// Creates a new uninitialized matrix or vector. - #[inline] - pub unsafe fn new_uninitialized($($args: usize),*) -> MaybeUninit { - Self::new_uninitialized_generic($($gargs),*) - } - /// Creates a matrix or vector with all its elements set to `elem`. /// /// # Example diff --git a/src/base/default_allocator.rs b/src/base/default_allocator.rs index aa324646..23c80153 100644 --- a/src/base/default_allocator.rs +++ b/src/base/default_allocator.rs @@ -4,7 +4,6 @@ //! heap-allocated buffers for matrices with at least one dimension unknown at compile-time. use std::cmp; -use std::mem; use std::ptr; #[cfg(all(feature = "alloc", not(feature = "std")))] @@ -39,11 +38,6 @@ impl Allocator, Const> type Buffer = ArrayStorage; type BufferUninit = ArrayStorage, R, C>; - #[inline] - unsafe fn allocate_uninitialized(_: Const, _: Const) -> MaybeUninit { - mem::MaybeUninit::::uninit() - } - #[inline] fn allocate_uninit(_: Const, _: Const) -> ArrayStorage, R, C> { // SAFETY: An uninitialized `[MaybeUninit<_>; _]` is valid. @@ -95,23 +89,12 @@ impl Allocator for DefaultAllocator { type Buffer = VecStorage; type BufferUninit = VecStorage, Dynamic, C>; - #[inline] - unsafe fn allocate_uninitialized(nrows: Dynamic, ncols: C) -> MaybeUninit { - let mut res = Vec::new(); - let length = nrows.value() * ncols.value(); - res.reserve_exact(length); - res.set_len(length); - - mem::MaybeUninit::new(VecStorage::new(nrows, ncols, res)) - } - #[inline] fn allocate_uninit(nrows: Dynamic, ncols: C) -> VecStorage, Dynamic, C> { let mut data = Vec::new(); let length = nrows.value() * ncols.value(); data.reserve_exact(length); data.resize_with(length, MaybeUninit::uninit); - VecStorage::new(nrows, ncols, data) } @@ -153,16 +136,6 @@ impl Allocator for DefaultAllocator { type Buffer = VecStorage; type BufferUninit = VecStorage, R, Dynamic>; - #[inline] - unsafe fn allocate_uninitialized(nrows: R, ncols: Dynamic) -> MaybeUninit { - let mut res = Vec::new(); - let length = nrows.value() * ncols.value(); - res.reserve_exact(length); - res.set_len(length); - - mem::MaybeUninit::new(VecStorage::new(nrows, ncols, res)) - } - #[inline] fn allocate_uninit(nrows: R, ncols: Dynamic) -> VecStorage, R, Dynamic> { let mut data = Vec::new(); @@ -222,25 +195,21 @@ where unsafe fn reallocate_copy( rto: Const, cto: Const, - buf: >::Buffer, + mut buf: >::Buffer, ) -> ArrayStorage, RTO, CTO> { - #[cfg(feature = "no_unsound_assume_init")] - let mut res: ArrayStorage = unimplemented!(); - #[cfg(not(feature = "no_unsound_assume_init"))] - let mut res = - , Const>>::allocate_uninitialized(rto, cto) - .assume_init(); let mut res = , Const>>::allocate_uninit(rto, cto); let (rfrom, cfrom) = buf.shape(); let len_from = rfrom.value() * cfrom.value(); let len_to = rto.value() * cto.value(); - ptr::copy_nonoverlapping( - buf.ptr(), - res.ptr_mut() as *mut T, - cmp::min(len_from, len_to), - ); + let len_copied = cmp::min(len_from, len_to); + ptr::copy_nonoverlapping(buf.ptr(), res.ptr_mut() as *mut T, len_copied); + + // Safety: + // - We don’t care about dropping elements because the caller is responsible for dropping things. + // - We forget `buf` so that we don’t drop the other elements. + std::mem::forget(buf); res } @@ -257,7 +226,7 @@ where unsafe fn reallocate_copy( rto: Dynamic, cto: CTo, - buf: ArrayStorage, + mut buf: ArrayStorage, ) -> VecStorage, Dynamic, CTo> { let mut res = >::allocate_uninit(rto, cto); @@ -265,11 +234,13 @@ where let len_from = rfrom.value() * cfrom.value(); let len_to = rto.value() * cto.value(); - ptr::copy_nonoverlapping( - buf.ptr(), - res.ptr_mut() as *mut T, - cmp::min(len_from, len_to), - ); + let len_copied = cmp::min(len_from, len_to); + ptr::copy_nonoverlapping(buf.ptr(), res.ptr_mut() as *mut T, len_copied); + + // Safety: + // - We don’t care about dropping elements because the caller is responsible for dropping things. + // - We forget `buf` so that we don’t drop the other elements. + std::mem::forget(buf); res } @@ -286,7 +257,7 @@ where unsafe fn reallocate_copy( rto: RTo, cto: Dynamic, - buf: ArrayStorage, + mut buf: ArrayStorage, ) -> VecStorage, RTo, Dynamic> { let mut res = >::allocate_uninit(rto, cto); @@ -294,11 +265,13 @@ where let len_from = rfrom.value() * cfrom.value(); let len_to = rto.value() * cto.value(); - ptr::copy_nonoverlapping( - buf.ptr(), - res.ptr_mut() as *mut T, - cmp::min(len_from, len_to), - ); + let len_copied = cmp::min(len_from, len_to); + ptr::copy_nonoverlapping(buf.ptr(), res.ptr_mut() as *mut T, len_copied); + + // Safety: + // - We don’t care about dropping elements because the caller is responsible for dropping things. + // - We forget `buf` so that we don’t drop the other elements. + std::mem::forget(buf); res } diff --git a/src/base/edition.rs b/src/base/edition.rs index 5832d80b..bca017c4 100644 --- a/src/base/edition.rs +++ b/src/base/edition.rs @@ -369,12 +369,23 @@ impl> Matrix { let mut target: usize = 0; while offset + target < ncols.value() { if indices.contains(&(target + offset)) { + // Safety: the resulting pointer is within range. + let col_ptr = unsafe { m.data.ptr_mut().add((target + offset) * nrows.value()) }; + // Drop every element in the column we are about to overwrite. + // We use the a similar technique as in `Vec::truncate`. + let s = ptr::slice_from_raw_parts_mut(col_ptr, nrows.value()); + // Safety: we drop the column in-place, which is OK because we will overwrite these + // entries later in the loop, or discard them with the `reallocate_copy` + // afterwards. + unsafe { ptr::drop_in_place(s) }; + offset += 1; } else { unsafe { let ptr_source = m.data.ptr().add((target + offset) * nrows.value()); let ptr_target = m.data.ptr_mut().add(target * nrows.value()); + // Copy the data, overwriting what we dropped. ptr::copy(ptr_source, ptr_target, nrows.value()); target += 1; } @@ -409,12 +420,21 @@ impl> Matrix { let mut target: usize = 0; while offset + target < nrows.value() * ncols.value() { if indices.contains(&((target + offset) % nrows.value())) { + // Safety: the resulting pointer is within range. + unsafe { + let elt_ptr = m.data.ptr_mut().add(target + offset); + // Safety: we drop the component in-place, which is OK because we will overwrite these + // entries later in the loop, or discard them with the `reallocate_copy` + // afterwards. + ptr::drop_in_place(elt_ptr) + }; offset += 1; } else { unsafe { let ptr_source = m.data.ptr().add(target + offset); let ptr_target = m.data.ptr_mut().add(target); + // Copy the data, overwriting what we dropped in the previous iterations. ptr::copy(ptr_source, ptr_target, 1); target += 1; } @@ -479,7 +499,8 @@ impl> Matrix { "Column index out of range." ); - if nremove.value() != 0 && i + nremove.value() < ncols.value() { + let need_column_shifts = nremove.value() != 0 && i + nremove.value() < ncols.value(); + if need_column_shifts { // The first `deleted_i * nrows` are left untouched. let copied_value_start = i + nremove.value(); @@ -487,12 +508,26 @@ impl> Matrix { let ptr_in = m.data.ptr().add(copied_value_start * nrows.value()); let ptr_out = m.data.ptr_mut().add(i * nrows.value()); + // Drop all the elements of the columns we are about to overwrite. + // We use the a similar technique as in `Vec::truncate`. + let s = ptr::slice_from_raw_parts_mut(ptr_out, nremove.value() * nrows.value()); + // Safety: we drop the column in-place, which is OK because we will overwrite these + // entries with `ptr::copy` afterward. + ptr::drop_in_place(s); + ptr::copy( ptr_in, ptr_out, (ncols.value() - copied_value_start) * nrows.value(), ); } + } else { + // All the columns to remove are at the end of the buffer. Drop them. + unsafe { + let ptr = m.data.ptr_mut().add(i * nrows.value()); + let s = ptr::slice_from_raw_parts_mut(ptr, nremove.value() * nrows.value()); + ptr::drop_in_place(s) + }; } // Safety: The new size is smaller than the old size, so @@ -844,8 +879,21 @@ impl> Matrix { let mut data = self.into_owned(); if new_nrows.value() == nrows { + if new_ncols.value() < ncols { + unsafe { + let num_cols_to_delete = ncols - new_ncols.value(); + let col_ptr = data.data.ptr_mut().add(new_ncols.value() * nrows); + let s = ptr::slice_from_raw_parts_mut(col_ptr, num_cols_to_delete * nrows); + // Safety: drop the elements of the deleted columns. + // these are the elements that will be truncated + // by the `reallocate_copy` afterward. + ptr::drop_in_place(s) + }; + } + let res = unsafe { DefaultAllocator::reallocate_copy(new_nrows, new_ncols, data.data) }; let mut res = Matrix::from_data(res); + if new_ncols.value() > ncols { res.columns_range_mut(ncols..) .fill_with(|| MaybeUninit::new(val.inlined_clone())); @@ -1027,6 +1075,10 @@ where } } +// Move the elements of `data` in such a way that the matrix with +// the rows `[i, i + nremove[` deleted is represented in a contigous +// way in `data` after this method completes. +// Every deleted element are manually dropped by this method. unsafe fn compress_rows( data: &mut [T], nrows: usize, @@ -1036,16 +1088,28 @@ unsafe fn compress_rows( ) { let new_nrows = nrows - nremove; - if new_nrows == 0 || ncols == 0 { - return; // Nothing to do as the output matrix is empty. + if nremove == 0 { + return; // Nothing to remove or drop. } + if new_nrows == 0 || ncols == 0 { + // The output matrix is empty, drop everything. + ptr::drop_in_place(data.as_mut()); + return; + } + + // Safety: because `nremove != 0`, the pointers given to `ptr::copy` + // won’t alias. let ptr_in = data.as_ptr(); let ptr_out = data.as_mut_ptr(); let mut curr_i = i; for k in 0..ncols - 1 { + // Safety: we drop the row elements in-place because we will overwrite these + // entries later with the `ptr::copy`. + let s = ptr::slice_from_raw_parts_mut(ptr_out.add(curr_i), nremove); + ptr::drop_in_place(s); ptr::copy( ptr_in.add(curr_i + (k + 1) * nremove), ptr_out.add(curr_i), @@ -1055,7 +1119,13 @@ unsafe fn compress_rows( curr_i += new_nrows; } - // Deal with the last column from which less values have to be copied. + /* + * Deal with the last column from which less values have to be copied. + */ + // Safety: we drop the row elements in-place because we will overwrite these + // entries later with the `ptr::copy`. + let s = ptr::slice_from_raw_parts_mut(ptr_out.add(curr_i), nremove); + ptr::drop_in_place(s); let remaining_len = nrows - i - nremove; ptr::copy( ptr_in.add(nrows * ncols - remaining_len), diff --git a/src/base/matrix.rs b/src/base/matrix.rs index e9d655be..6e868354 100644 --- a/src/base/matrix.rs +++ b/src/base/matrix.rs @@ -436,20 +436,6 @@ impl> Matrix { unsafe { Self::from_data_statically_unchecked(data) } } - /// Creates a new uninitialized matrix with the given uninitialized data - pub unsafe fn from_uninitialized_data(data: MaybeUninit) -> MaybeUninit { - let res: Matrix> = Matrix { - data, - _phantoms: PhantomData, - }; - let res: MaybeUninit>> = MaybeUninit::new(res); - // safety: since we wrap the inner MaybeUninit in an outer MaybeUninit above, the fact that the `data` field is partially-uninitialized is still opaque. - // with s/transmute_copy/transmute/, rustc claims that `MaybeUninit>>` may be of a different size from `MaybeUninit>` - // but MaybeUninit's documentation says "MaybeUninit is guaranteed to have the same size, alignment, and ABI as T", which implies those types should be the same size - let res: MaybeUninit> = mem::transmute_copy(&res); - res - } - /// The shape of this matrix returned as the tuple (number of rows, number of columns). /// /// # Examples: @@ -1209,7 +1195,7 @@ impl> Matrix { } } -impl> Matrix { +impl> Matrix { /// Returns a mutable pointer to the start of the matrix. /// /// If the matrix is not empty, this pointer is guaranteed to be aligned @@ -1246,7 +1232,10 @@ impl> Matrix { /// /// The components of the slice are assumed to be ordered in column-major order. #[inline] - pub fn copy_from_slice(&mut self, slice: &[T]) { + pub fn copy_from_slice(&mut self, slice: &[T]) + where + T: Scalar, + { let (nrows, ncols) = self.shape(); assert!( @@ -1268,6 +1257,7 @@ impl> Matrix { #[inline] pub fn copy_from(&mut self, other: &Matrix) where + T: Scalar, R2: Dim, C2: Dim, SB: RawStorage, @@ -1291,6 +1281,7 @@ impl> Matrix { #[inline] pub fn tr_copy_from(&mut self, other: &Matrix) where + T: Scalar, R2: Dim, C2: Dim, SB: RawStorage, diff --git a/src/base/vec_storage.rs b/src/base/vec_storage.rs index a34f8d88..bf73661d 100644 --- a/src/base/vec_storage.rs +++ b/src/base/vec_storage.rs @@ -113,14 +113,17 @@ impl VecStorage { /// Resizes the underlying mutable data storage and unwraps it. /// /// # Safety - /// If `sz` is larger than the current size, additional elements are uninitialized. - /// If `sz` is smaller than the current size, additional elements are truncated. + /// - If `sz` is larger than the current size, additional elements are uninitialized. + /// - If `sz` is smaller than the current size, additional elements are truncated but **not** dropped. + /// It is the responsibility of the caller of this method to drop these elements. #[inline] pub unsafe fn resize(mut self, sz: usize) -> Vec> { let len = self.len(); - if sz < len { - self.data.truncate(sz); + let new_data = if sz < len { + // Use `set_len` instead of `truncate` because we don’t want to + // drop the removed elements (it’s the caller’s responsibility). + self.data.set_len(sz); self.data.shrink_to_fit(); // Safety: @@ -147,7 +150,12 @@ impl VecStorage { // to be initialized. new_data.set_len(sz); new_data - } + }; + + // Avoid double-free by forgetting `self` because its data buffer has + // been transfered to `new_data`. + std::mem::forget(self); + new_data } /// The number of elements on the underlying vector. diff --git a/src/third_party/mint/mint_matrix.rs b/src/third_party/mint/mint_matrix.rs index 73d0a936..ce45fcda 100644 --- a/src/third_party/mint/mint_matrix.rs +++ b/src/third_party/mint/mint_matrix.rs @@ -1,9 +1,9 @@ use std::convert::{AsMut, AsRef, From, Into}; -use std::mem; +use std::mem::{self, MaybeUninit}; use std::ptr; use crate::base::allocator::Allocator; -use crate::base::dimension::{U1, U2, U3, U4}; +use crate::base::dimension::{Const, DimName, U1, U2, U3, U4}; use crate::base::storage::{IsContiguous, RawStorage, RawStorageMut}; use crate::base::{DefaultAllocator, Matrix, OMatrix, Scalar}; @@ -15,9 +15,12 @@ macro_rules! impl_from_into_mint_1D( #[inline] fn from(v: mint::$VT) -> Self { unsafe { - let mut res = Self::new_uninitialized(); - ptr::copy_nonoverlapping(&v.x, (*res.as_mut_ptr()).data.ptr_mut(), $SZ); - + let mut res = Matrix::uninit(<$NRows>::name(), Const::<1>); + // Copy the data. + ptr::copy_nonoverlapping(&v.x, res.data.ptr_mut() as *mut T, $SZ); + // Prevent from being dropped the originals we just copied. + mem::forget(v); + // The result is now fully initialized. res.assume_init() } } @@ -30,9 +33,13 @@ macro_rules! impl_from_into_mint_1D( fn into(self) -> mint::$VT { // SAFETY: this is OK thanks to the IsContiguous bound. unsafe { - let mut res: mint::$VT = mem::MaybeUninit::uninit().assume_init(); - ptr::copy_nonoverlapping(self.data.ptr(), &mut res.x, $SZ); - res + let mut res: MaybeUninit> = MaybeUninit::uninit(); + // Copy the data. + ptr::copy_nonoverlapping(self.data.ptr(), res.as_mut_ptr() as *mut T, $SZ); + // Prevent from being dropped the originals we just copied. + mem::forget(self); + // The result is now fully initialized. + res.assume_init() } } } @@ -78,13 +85,15 @@ macro_rules! impl_from_into_mint_2D( #[inline] fn from(m: mint::$MV) -> Self { unsafe { - let mut res = Self::new_uninitialized(); - let mut ptr = (*res.as_mut_ptr()).data.ptr_mut(); + let mut res = Matrix::uninit(<$NRows>::name(), <$NCols>::name()); + let mut ptr = res.data.ptr_mut(); $( - ptr::copy_nonoverlapping(&m.$component.x, ptr, $SZRows); + ptr::copy_nonoverlapping(&m.$component.x, ptr as *mut T, $SZRows); ptr = ptr.offset($SZRows); )* - let _ = ptr; + let _ = ptr; // Just to avoid some unused assignment warnings. + // Forget the original data to avoid double-free. + mem::forget(m); res.assume_init() } } @@ -96,14 +105,16 @@ macro_rules! impl_from_into_mint_2D( #[inline] fn into(self) -> mint::$MV { unsafe { - let mut res: mint::$MV = mem::MaybeUninit::uninit().assume_init(); + let mut res: MaybeUninit> = MaybeUninit::uninit(); let mut ptr = self.data.ptr(); $( - ptr::copy_nonoverlapping(ptr, &mut res.$component.x, $SZRows); + ptr::copy_nonoverlapping(ptr, ptr::addr_of_mut!((*res.as_mut_ptr()).$component) as *mut T, $SZRows); ptr = ptr.offset($SZRows); )* let _ = ptr; - res + // Forget the original data to avoid double-free. + mem::forget(self); + res.assume_init() } } }