Fix potential undoundness with Storage::as_slice and Storage::as_mut_slice (#905)

sebcrozet · web-flow · commit 38add0b00df2 · 2021-06-17T09:46:49.000+02:00
diff --git a/src/base/array_storage.rs b/src/base/array_storage.rs
@@ -79,7 +79,7 @@ where
     }
 
     #[inline]
-    fn is_contiguous(&self) -> bool {
+    unsafe fn is_contiguous(&self) -> bool {
         true
     }
 
@@ -101,8 +101,8 @@ where
     }
 
     #[inline]
-    fn as_slice(&self) -> &[T] {
-        unsafe { std::slice::from_raw_parts(self.ptr(), R * C) }
+    unsafe fn as_slice_unchecked(&self) -> &[T] {
+        std::slice::from_raw_parts(self.ptr(), R * C)
     }
 }
 
@@ -118,8 +118,8 @@ where
     }
 
     #[inline]
-    fn as_mut_slice(&mut self) -> &mut [T] {
-        unsafe { std::slice::from_raw_parts_mut(self.ptr_mut(), R * C) }
+    unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {
+        std::slice::from_raw_parts_mut(self.ptr_mut(), R * C)
     }
 }
 
diff --git a/src/base/blas.rs b/src/base/blas.rs
@@ -344,13 +344,17 @@ where
         let rstride1 = self.strides().0;
         let rstride2 = x.strides().0;
 
-        let y = self.data.as_mut_slice();
-        let x = x.data.as_slice();
+        unsafe {
+            // SAFETY: the conversion to slices is OK because we access the
+            //         elements taking the strides into account.
+            let y = self.data.as_mut_slice_unchecked();
+            let x = x.data.as_slice_unchecked();
 
-        if !b.is_zero() {
-            array_axcpy(y, a, x, c, b, rstride1, rstride2, x.len());
-        } else {
-            array_axc(y, a, x, c, rstride1, rstride2, x.len());
+            if !b.is_zero() {
+                array_axcpy(y, a, x, c, b, rstride1, rstride2, x.len());
+            } else {
+                array_axc(y, a, x, c, rstride1, rstride2, x.len());
+            }
         }
     }
 
diff --git a/src/base/default_allocator.rs b/src/base/default_allocator.rs
@@ -16,7 +16,7 @@ use crate::base::array_storage::ArrayStorage;
 #[cfg(any(feature = "alloc", feature = "std"))]
 use crate::base::dimension::Dynamic;
 use crate::base::dimension::{Dim, DimName};
-use crate::base::storage::{Storage, StorageMut};
+use crate::base::storage::{ContiguousStorageMut, Storage, StorageMut};
 #[cfg(any(feature = "std", feature = "alloc"))]
 use crate::base::vec_storage::VecStorage;
 use crate::base::Scalar;
diff --git a/src/base/edition.rs b/src/base/edition.rs
@@ -11,7 +11,7 @@ use crate::base::constraint::{DimEq, SameNumberOfColumns, SameNumberOfRows, Shap
 #[cfg(any(feature = "std", feature = "alloc"))]
 use crate::base::dimension::Dynamic;
 use crate::base::dimension::{Const, Dim, DimAdd, DimDiff, DimMin, DimMinimum, DimSub, DimSum, U1};
-use crate::base::storage::{ReshapableStorage, Storage, StorageMut};
+use crate::base::storage::{ContiguousStorageMut, ReshapableStorage, Storage, StorageMut};
 use crate::base::{DefaultAllocator, Matrix, OMatrix, RowVector, Scalar, Vector};
 
 /// # Rows and columns extraction
diff --git a/src/base/matrix_slice.rs b/src/base/matrix_slice.rs
@@ -132,7 +132,7 @@ macro_rules! storage_impl(
             }
 
             #[inline]
-            fn is_contiguous(&self) -> bool {
+            unsafe fn is_contiguous(&self) -> bool {
                 // Common cases that can be deduced at compile-time even if one of the dimensions
                 // is Dynamic.
                 if (RStride::is::<U1>() && C::is::<U1>()) || // Column vector.
@@ -162,14 +162,14 @@ macro_rules! storage_impl(
             }
 
             #[inline]
-            fn as_slice(&self) -> &[T] {
+            unsafe fn as_slice_unchecked(&self) -> &[T] {
                 let (nrows, ncols) = self.shape();
                 if nrows.value() != 0 && ncols.value() != 0 {
                     let sz = self.linear_index(nrows.value() - 1, ncols.value() - 1);
-                    unsafe { slice::from_raw_parts(self.ptr, sz + 1) }
+                    slice::from_raw_parts(self.ptr, sz + 1)
                 }
                 else {
-                    unsafe { slice::from_raw_parts(self.ptr, 0) }
+                    slice::from_raw_parts(self.ptr, 0)
                 }
             }
         }
@@ -187,13 +187,13 @@ unsafe impl<'a, T: Scalar, R: Dim, C: Dim, RStride: Dim, CStride: Dim> StorageMu
     }
 
     #[inline]
-    fn as_mut_slice(&mut self) -> &mut [T] {
+    unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {
         let (nrows, ncols) = self.shape();
         if nrows.value() != 0 && ncols.value() != 0 {
             let sz = self.linear_index(nrows.value() - 1, ncols.value() - 1);
-            unsafe { slice::from_raw_parts_mut(self.ptr, sz + 1) }
+            slice::from_raw_parts_mut(self.ptr, sz + 1)
         } else {
-            unsafe { slice::from_raw_parts_mut(self.ptr, 0) }
+            slice::from_raw_parts_mut(self.ptr, 0)
         }
     }
 }
diff --git a/src/base/ops.rs b/src/base/ops.rs
@@ -158,20 +158,17 @@ macro_rules! componentwise_binop_impl(
 
                 // This is the most common case and should be deduced at compile-time.
                 // TODO: use specialization instead?
-                if self.data.is_contiguous() && rhs.data.is_contiguous() && out.data.is_contiguous() {
-                    let arr1 = self.data.as_slice();
-                    let arr2 = rhs.data.as_slice();
-                    let out  = out.data.as_mut_slice();
-                    for i in 0 .. arr1.len() {
-                        unsafe {
+                unsafe {
+                    if self.data.is_contiguous() && rhs.data.is_contiguous() && out.data.is_contiguous() {
+                        let arr1 = self.data.as_slice_unchecked();
+                        let arr2 = rhs.data.as_slice_unchecked();
+                        let out  = out.data.as_mut_slice_unchecked();
+                        for i in 0 .. arr1.len() {
                             *out.get_unchecked_mut(i) = arr1.get_unchecked(i).inlined_clone().$method(arr2.get_unchecked(i).inlined_clone());
                         }
-                    }
-                }
-                else {
-                    for j in 0 .. self.ncols() {
-                        for i in 0 .. self.nrows() {
-                            unsafe {
+                    } else {
+                        for j in 0 .. self.ncols() {
+                            for i in 0 .. self.nrows() {
                                 let val = self.get_unchecked((i, j)).inlined_clone().$method(rhs.get_unchecked((i, j)).inlined_clone());
                                 *out.get_unchecked_mut((i, j)) = val;
                             }
@@ -191,19 +188,17 @@ macro_rules! componentwise_binop_impl(
 
                 // This is the most common case and should be deduced at compile-time.
                 // TODO: use specialization instead?
-                if self.data.is_contiguous() && rhs.data.is_contiguous() {
-                    let arr1 = self.data.as_mut_slice();
-                    let arr2 = rhs.data.as_slice();
-                    for i in 0 .. arr2.len() {
-                        unsafe {
+                unsafe {
+                    if self.data.is_contiguous() && rhs.data.is_contiguous() {
+                        let arr1 = self.data.as_mut_slice_unchecked();
+                        let arr2 = rhs.data.as_slice_unchecked();
+
+                        for i in 0 .. arr2.len() {
                             arr1.get_unchecked_mut(i).$method_assign(arr2.get_unchecked(i).inlined_clone());
                         }
-                    }
-                }
-                else {
-                    for j in 0 .. rhs.ncols() {
-                        for i in 0 .. rhs.nrows() {
-                            unsafe {
+                    } else {
+                        for j in 0 .. rhs.ncols() {
+                            for i in 0 .. rhs.nrows() {
                                 self.get_unchecked_mut((i, j)).$method_assign(rhs.get_unchecked((i, j)).inlined_clone())
                             }
                         }
@@ -221,20 +216,18 @@ macro_rules! componentwise_binop_impl(
 
                 // This is the most common case and should be deduced at compile-time.
                 // TODO: use specialization instead?
-                if self.data.is_contiguous() && rhs.data.is_contiguous() {
-                    let arr1 = self.data.as_slice();
-                    let arr2 = rhs.data.as_mut_slice();
-                    for i in 0 .. arr1.len() {
-                        unsafe {
+                unsafe {
+                    if self.data.is_contiguous() && rhs.data.is_contiguous() {
+                        let arr1 = self.data.as_slice_unchecked();
+                        let arr2 = rhs.data.as_mut_slice_unchecked();
+
+                        for i in 0 .. arr1.len() {
                             let res = arr1.get_unchecked(i).inlined_clone().$method(arr2.get_unchecked(i).inlined_clone());
                             *arr2.get_unchecked_mut(i) = res;
                         }
-                    }
-                }
-                else {
-                    for j in 0 .. self.ncols() {
-                        for i in 0 .. self.nrows() {
-                            unsafe {
+                    } else {
+                        for j in 0 .. self.ncols() {
+                            for i in 0 .. self.nrows() {
                                 let r = rhs.get_unchecked_mut((i, j));
                                 *r = self.get_unchecked((i, j)).inlined_clone().$method(r.inlined_clone())
                             }
diff --git a/src/base/storage.rs b/src/base/storage.rs
@@ -94,12 +94,19 @@ pub unsafe trait Storage<T: Scalar, R: Dim, C: Dim = U1>: Debug + Sized {
     }
 
     /// Indicates whether this data buffer stores its elements contiguously.
-    fn is_contiguous(&self) -> bool;
+    ///
+    /// This method is unsafe because unsafe code relies on this properties to performe
+    /// some low-lever optimizations.
+    unsafe fn is_contiguous(&self) -> bool;
 
     /// Retrieves the data buffer as a contiguous slice.
     ///
     /// The matrix components may not be stored in a contiguous way, depending on the strides.
-    fn as_slice(&self) -> &[T];
+    /// This method is unsafe because this can yield to invalid aliasing when called on some pairs
+    /// of matrix slices originating from the same matrix with strides.
+    ///
+    /// Call the safe alternative `matrix.as_slice()` instead.
+    unsafe fn as_slice_unchecked(&self) -> &[T];
 
     /// Builds a matrix data storage that does not contain any reference.
     fn into_owned(self) -> Owned<T, R, C>
@@ -165,8 +172,12 @@ pub unsafe trait StorageMut<T: Scalar, R: Dim, C: Dim = U1>: Storage<T, R, C> {
 
     /// Retrieves the mutable data buffer as a contiguous slice.
     ///
-    /// Matrix components may not be contiguous, depending on its strides.
-    fn as_mut_slice(&mut self) -> &mut [T];
+    /// Matrix components may not be contiguous, depending on its strides.    
+    ///
+    /// The matrix components may not be stored in a contiguous way, depending on the strides.
+    /// This method is unsafe because this can yield to invalid aliasing when called on some pairs
+    /// of matrix slices originating from the same matrix with strides.
+    unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T];
 }
 
 /// A matrix storage that is stored contiguously in memory.
@@ -177,6 +188,12 @@ pub unsafe trait StorageMut<T: Scalar, R: Dim, C: Dim = U1>: Storage<T, R, C> {
 pub unsafe trait ContiguousStorage<T: Scalar, R: Dim, C: Dim = U1>:
     Storage<T, R, C>
 {
+    /// Converts this data storage to a contiguous slice.
+    fn as_slice(&self) -> &[T] {
+        // SAFETY: this is safe because this trait guarantees the fact
+        //         that the data is stored contiguously.
+        unsafe { self.as_slice_unchecked() }
+    }
 }
 
 /// A mutable matrix storage that is stored contiguously in memory.
@@ -187,6 +204,12 @@ pub unsafe trait ContiguousStorage<T: Scalar, R: Dim, C: Dim = U1>:
 pub unsafe trait ContiguousStorageMut<T: Scalar, R: Dim, C: Dim = U1>:
     ContiguousStorage<T, R, C> + StorageMut<T, R, C>
 {
+    /// Converts this data storage to a contiguous mutable slice.
+    fn as_mut_slice(&mut self) -> &mut [T] {
+        // SAFETY: this is safe because this trait guarantees the fact
+        //         that the data is stored contiguously.
+        unsafe { self.as_mut_slice_unchecked() }
+    }
 }
 
 /// A matrix storage that can be reshaped in-place.
diff --git a/src/base/vec_storage.rs b/src/base/vec_storage.rs
@@ -178,7 +178,7 @@ where
     }
 
     #[inline]
-    fn is_contiguous(&self) -> bool {
+    unsafe fn is_contiguous(&self) -> bool {
         true
     }
 
@@ -199,7 +199,7 @@ where
     }
 
     #[inline]
-    fn as_slice(&self) -> &[T] {
+    unsafe fn as_slice_unchecked(&self) -> &[T] {
         &self.data
     }
 }
@@ -227,7 +227,7 @@ where
     }
 
     #[inline]
-    fn is_contiguous(&self) -> bool {
+    unsafe fn is_contiguous(&self) -> bool {
         true
     }
 
@@ -248,7 +248,7 @@ where
     }
 
     #[inline]
-    fn as_slice(&self) -> &[T] {
+    unsafe fn as_slice_unchecked(&self) -> &[T] {
         &self.data
     }
 }
@@ -268,7 +268,7 @@ where
     }
 
     #[inline]
-    fn as_mut_slice(&mut self) -> &mut [T] {
+    unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {
         &mut self.data[..]
     }
 }
@@ -329,7 +329,7 @@ where
     }
 
     #[inline]
-    fn as_mut_slice(&mut self) -> &mut [T] {
+    unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {
         &mut self.data[..]
     }
 }
diff --git a/src/linalg/inverse.rs b/src/linalg/inverse.rs
@@ -127,7 +127,7 @@ fn do_inverse4<T: ComplexField, D: Dim, S: StorageMut<T, D, D>>(
 where
     DefaultAllocator: Allocator<T, D, D>,
 {
-    let m = m.data.as_slice();
+    let m = m.as_slice();
 
     out[(0, 0)] = m[5] * m[10] * m[15] - m[5] * m[11] * m[14] - m[9] * m[6] * m[15]
         + m[9] * m[7] * m[14]

Original file line number	Diff line number	Diff line change
`@@ -79,7 +79,7 @@ where`
`79`	`79`	`}`
`80`	`80`
`81`	`81`	`#[inline]`
`82`		`- fn is_contiguous(&self) -> bool {`
	`82`	`+ unsafe fn is_contiguous(&self) -> bool {`
`83`	`83`	`true`
`84`	`84`	`}`
`85`	`85`
`@@ -101,8 +101,8 @@ where`
`101`	`101`	`}`
`102`	`102`
`103`	`103`	`#[inline]`
`104`		`- fn as_slice(&self) -> &[T] {`
`105`		`- unsafe { std::slice::from_raw_parts(self.ptr(), R * C) }`
	`104`	`+ unsafe fn as_slice_unchecked(&self) -> &[T] {`
	`105`	`+ std::slice::from_raw_parts(self.ptr(), R * C)`
`106`	`106`	`}`
`107`	`107`	`}`
`108`	`108`
`@@ -118,8 +118,8 @@ where`
`118`	`118`	`}`
`119`	`119`
`120`	`120`	`#[inline]`
`121`		`- fn as_mut_slice(&mut self) -> &mut [T] {`
`122`		`- unsafe { std::slice::from_raw_parts_mut(self.ptr_mut(), R * C) }`
	`121`	`+ unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {`
	`122`	`+ std::slice::from_raw_parts_mut(self.ptr_mut(), R * C)`
`123`	`123`	`}`
`124`	`124`	`}`
`125`	`125`
Original file line number	Diff line number	Diff line change
`@@ -132,7 +132,7 @@ macro_rules! storage_impl(`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`#[inline]`
`135`		`- fn is_contiguous(&self) -> bool {`
	`135`	`+ unsafe fn is_contiguous(&self) -> bool {`
`136`	`136`	`// Common cases that can be deduced at compile-time even if one of the dimensions`
`137`	`137`	`// is Dynamic.`
`138`	`138`	`if (RStride::is::<U1>() && C::is::<U1>()) \|\| // Column vector.`
`@@ -162,14 +162,14 @@ macro_rules! storage_impl(`
`162`	`162`	`}`
`163`	`163`
`164`	`164`	`#[inline]`
`165`		`- fn as_slice(&self) -> &[T] {`
	`165`	`+ unsafe fn as_slice_unchecked(&self) -> &[T] {`
`166`	`166`	`let (nrows, ncols) = self.shape();`
`167`	`167`	`if nrows.value() != 0 && ncols.value() != 0 {`
`168`	`168`	`let sz = self.linear_index(nrows.value() - 1, ncols.value() - 1);`
`169`		`- unsafe { slice::from_raw_parts(self.ptr, sz + 1) }`
	`169`	`+ slice::from_raw_parts(self.ptr, sz + 1)`
`170`	`170`	`}`
`171`	`171`	`else {`
`172`		`- unsafe { slice::from_raw_parts(self.ptr, 0) }`
	`172`	`+ slice::from_raw_parts(self.ptr, 0)`
`173`	`173`	`}`
`174`	`174`	`}`
`175`	`175`	`}`
`@@ -187,13 +187,13 @@ unsafe impl<'a, T: Scalar, R: Dim, C: Dim, RStride: Dim, CStride: Dim> StorageMu`
`187`	`187`	`}`
`188`	`188`
`189`	`189`	`#[inline]`
`190`		`- fn as_mut_slice(&mut self) -> &mut [T] {`
	`190`	`+ unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {`
`191`	`191`	`let (nrows, ncols) = self.shape();`
`192`	`192`	`if nrows.value() != 0 && ncols.value() != 0 {`
`193`	`193`	`let sz = self.linear_index(nrows.value() - 1, ncols.value() - 1);`
`194`		`- unsafe { slice::from_raw_parts_mut(self.ptr, sz + 1) }`
	`194`	`+ slice::from_raw_parts_mut(self.ptr, sz + 1)`
`195`	`195`	`} else {`
`196`		`- unsafe { slice::from_raw_parts_mut(self.ptr, 0) }`
	`196`	`+ slice::from_raw_parts_mut(self.ptr, 0)`
`197`	`197`	`}`
`198`	`198`	`}`
`199`	`199`	`}`
Original file line number	Diff line number	Diff line change
`@@ -178,7 +178,7 @@ where`
`178`	`178`	`}`
`179`	`179`
`180`	`180`	`#[inline]`
`181`		`- fn is_contiguous(&self) -> bool {`
	`181`	`+ unsafe fn is_contiguous(&self) -> bool {`
`182`	`182`	`true`
`183`	`183`	`}`
`184`	`184`
`@@ -199,7 +199,7 @@ where`
`199`	`199`	`}`
`200`	`200`
`201`	`201`	`#[inline]`
`202`		`- fn as_slice(&self) -> &[T] {`
	`202`	`+ unsafe fn as_slice_unchecked(&self) -> &[T] {`
`203`	`203`	`&self.data`
`204`	`204`	`}`
`205`	`205`	`}`
`@@ -227,7 +227,7 @@ where`
`227`	`227`	`}`
`228`	`228`
`229`	`229`	`#[inline]`
`230`		`- fn is_contiguous(&self) -> bool {`
	`230`	`+ unsafe fn is_contiguous(&self) -> bool {`
`231`	`231`	`true`
`232`	`232`	`}`
`233`	`233`
`@@ -248,7 +248,7 @@ where`
`248`	`248`	`}`
`249`	`249`
`250`	`250`	`#[inline]`
`251`		`- fn as_slice(&self) -> &[T] {`
	`251`	`+ unsafe fn as_slice_unchecked(&self) -> &[T] {`
`252`	`252`	`&self.data`
`253`	`253`	`}`
`254`	`254`	`}`
`@@ -268,7 +268,7 @@ where`
`268`	`268`	`}`
`269`	`269`
`270`	`270`	`#[inline]`
`271`		`- fn as_mut_slice(&mut self) -> &mut [T] {`
	`271`	`+ unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {`
`272`	`272`	`&mut self.data[..]`
`273`	`273`	`}`
`274`	`274`	`}`
`@@ -329,7 +329,7 @@ where`
`329`	`329`	`}`
`330`	`330`
`331`	`331`	`#[inline]`
`332`		`- fn as_mut_slice(&mut self) -> &mut [T] {`
	`332`	`+ unsafe fn as_mut_slice_unchecked(&mut self) -> &mut [T] {`
`333`	`333`	`&mut self.data[..]`
`334`	`334`	`}`
`335`	`335`	`}`
Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ fn do_inverse4<T: ComplexField, D: Dim, S: StorageMut<T, D, D>>(`
`127`	`127`	`where`
`128`	`128`	`DefaultAllocator: Allocator<T, D, D>,`
`129`	`129`	`{`
`130`		`- let m = m.data.as_slice();`
	`130`	`+ let m = m.as_slice();`
`131`	`131`
`132`	`132`	`out[(0, 0)] = m[5] * m[10] * m[15] - m[5] * m[11] * m[14] - m[9] * m[6] * m[15]`
`133`	`133`	`+ m[9] * m[7] * m[14]`