From 8dd673d48bb04145cd6ec1042aaf0cc95f1deca8 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 4 Aug 2019 11:20:19 -0400 Subject: [PATCH 1/2] Don't call 'offset' on a dangling pointer When creating a matrix with only one zero dimension, we end up with a matrix with a total size of zero, but a non-zero stride for elements. While such a matrix can never actually have any elements, we need to be careful with how we use the pointer associated with it. Since such a pointer will always be dangling, it can never be used with `ptr.offset`, which requires that the pointer be in-bounds or one passed the end of an allocation. Violating this results in undefined behavior. This commit adds in checks before the uses of `ptr.offset`. If we ever need to offset from a pointer when our actual allocation size is zero, we skip offsetting, and return the original pointer. This is fine because any actual use of the original or offsetted pointer would already be undefined behavior - we shoul never be trying to dereference the pointer associated with a zero-size matrix. This issue was caught be running `cargo miri test` on the project. --- src/base/iter.rs | 20 +++++++++++++++++++- src/base/storage.rs | 22 ++++++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/base/iter.rs b/src/base/iter.rs index 74e4f018..bad4e0be 100644 --- a/src/base/iter.rs +++ b/src/base/iter.rs @@ -27,12 +27,30 @@ macro_rules! iterator { let shape = storage.shape(); let strides = storage.strides(); let inner_offset = shape.0.value() * strides.0.value(); + let size = shape.0.value() * shape.1.value(); let ptr = storage.$ptr(); + // If we have a size of 0, 'ptr' must be + // dangling. Howver, 'inner_offset' might + // not be zero if only one dimension is zero, so + // we don't want to call 'offset'. + // This pointer will never actually get used + // if our size is '0', so it's fine to use + // 'ptr' for both the start and end. + let inner_end = if size == 0 { + ptr + } else { + // Safety: + // If 'size' is non-zero, we know that 'ptr' + // is not dangling, and 'inner_offset' must lie + // within the allocation + unsafe { ptr.offset(inner_offset as isize) } + }; + $Name { ptr: ptr, inner_ptr: ptr, - inner_end: unsafe { ptr.offset(inner_offset as isize) }, + inner_end, size: shape.0.value() * shape.1.value(), strides: strides, _phantoms: PhantomData, diff --git a/src/base/storage.rs b/src/base/storage.rs index 02941e47..80a6a2d8 100644 --- a/src/base/storage.rs +++ b/src/base/storage.rs @@ -72,7 +72,16 @@ pub unsafe trait Storage: Debug + Sized { /// Gets the address of the i-th matrix component without performing bound-checking. #[inline] unsafe fn get_address_unchecked_linear(&self, i: usize) -> *const N { - self.ptr().offset(i as isize) + let shape = self.shape(); + if shape.0.value() * shape.1.value() == 0 { + // If we have a zero-size matrix, our pointer must + // be dangling. Instead of calling 'offset', we + // just re-use our pointer, since actually using + // it would be undefined behavior + self.ptr() + } else { + self.ptr().offset(i as isize) + } } /// Gets the address of the i-th matrix component without performing bound-checking. @@ -124,7 +133,16 @@ pub unsafe trait StorageMut: Storage { /// Gets the mutable address of the i-th matrix component without performing bound-checking. #[inline] unsafe fn get_address_unchecked_linear_mut(&mut self, i: usize) -> *mut N { - self.ptr_mut().offset(i as isize) + let shape = self.shape(); + if shape.0.value() * shape.1.value() == 0 { + // If we have a zero-size matrix, our pointer must + // be dangling. Instead of calling 'offset', we + // just re-use our pointer, since actually using + // it would be undefined behavior + self.ptr_mut() + } else { + self.ptr_mut().offset(i as isize) + } } /// Gets the mutable address of the i-th matrix component without performing bound-checking. From df87769988f67045b96c1711eb048f383b78de2a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 19 Nov 2019 17:42:33 -0500 Subject: [PATCH 2/2] Switch to `wrapping_offset` instead of unsafe `offset` --- src/base/storage.rs | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/src/base/storage.rs b/src/base/storage.rs index 80a6a2d8..e7439552 100644 --- a/src/base/storage.rs +++ b/src/base/storage.rs @@ -72,16 +72,7 @@ pub unsafe trait Storage: Debug + Sized { /// Gets the address of the i-th matrix component without performing bound-checking. #[inline] unsafe fn get_address_unchecked_linear(&self, i: usize) -> *const N { - let shape = self.shape(); - if shape.0.value() * shape.1.value() == 0 { - // If we have a zero-size matrix, our pointer must - // be dangling. Instead of calling 'offset', we - // just re-use our pointer, since actually using - // it would be undefined behavior - self.ptr() - } else { - self.ptr().offset(i as isize) - } + self.ptr().wrapping_offset(i as isize) } /// Gets the address of the i-th matrix component without performing bound-checking. @@ -133,16 +124,7 @@ pub unsafe trait StorageMut: Storage { /// Gets the mutable address of the i-th matrix component without performing bound-checking. #[inline] unsafe fn get_address_unchecked_linear_mut(&mut self, i: usize) -> *mut N { - let shape = self.shape(); - if shape.0.value() * shape.1.value() == 0 { - // If we have a zero-size matrix, our pointer must - // be dangling. Instead of calling 'offset', we - // just re-use our pointer, since actually using - // it would be undefined behavior - self.ptr_mut() - } else { - self.ptr_mut().offset(i as isize) - } + self.ptr_mut().wrapping_offset(i as isize) } /// Gets the mutable address of the i-th matrix component without performing bound-checking.