From 09f8885b3b84bc9ba06652dc58383f0e3ab97445 Mon Sep 17 00:00:00 2001 From: Peter Jaszkowiak Date: Wed, 30 Nov 2022 23:25:07 -0700 Subject: [PATCH 1/2] debug assertions for `slice::split_at_unchecked`, `str::get_unchecked` --- library/core/src/slice/index.rs | 5 +-- library/core/src/slice/mod.rs | 8 +++- library/core/src/str/traits.rs | 66 +++++++++++++++++---------------- 3 files changed, 44 insertions(+), 35 deletions(-) diff --git a/library/core/src/slice/index.rs b/library/core/src/slice/index.rs index c295a0e06458d..f0e5ea53d7d70 100644 --- a/library/core/src/slice/index.rs +++ b/library/core/src/slice/index.rs @@ -371,12 +371,11 @@ unsafe impl const SliceIndex<[T]> for ops::Range { #[inline] unsafe fn get_unchecked(self, slice: *const [T]) -> *const [T] { - let this = ops::Range { start: self.start, end: self.end }; + let this = ops::Range { ..self }; // SAFETY: the caller guarantees that `slice` is not dangling, so it // cannot be longer than `isize::MAX`. They also guarantee that // `self` is in bounds of `slice` so `self` cannot overflow an `isize`, // so the call to `add` is safe. - unsafe { assert_unsafe_precondition!( "slice::get_unchecked requires that the range is within the slice", @@ -389,7 +388,7 @@ unsafe impl const SliceIndex<[T]> for ops::Range { #[inline] unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] { - let this = ops::Range { start: self.start, end: self.end }; + let this = ops::Range { ..self }; // SAFETY: see comments for `get_unchecked` above. unsafe { assert_unsafe_precondition!( diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index d93a3a57ecd27..9926bf7fbbb3f 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -1679,7 +1679,13 @@ impl [T] { let ptr = self.as_ptr(); // SAFETY: Caller has to check that `0 <= mid <= self.len()` - unsafe { (from_raw_parts(ptr, mid), from_raw_parts(ptr.add(mid), len - mid)) } + unsafe { + assert_unsafe_precondition!( + "slice::split_at_unchecked requires the index to be within the slice", + (mid: usize, len: usize) => mid <= len + ); + (from_raw_parts(ptr, mid), from_raw_parts(ptr.add(mid), len - mid)) + } } /// Divides one mutable slice into two at an index, without doing bounds checking. diff --git a/library/core/src/str/traits.rs b/library/core/src/str/traits.rs index d3ed811b1575b..a7a2b03774a8f 100644 --- a/library/core/src/str/traits.rs +++ b/library/core/src/str/traits.rs @@ -1,6 +1,7 @@ //! Trait implementations for `str`. use crate::cmp::Ordering; +use crate::intrinsics::assert_unsafe_precondition; use crate::ops; use crate::ptr; use crate::slice::SliceIndex; @@ -198,7 +199,15 @@ unsafe impl const SliceIndex for ops::Range { let slice = slice as *const [u8]; // SAFETY: the caller guarantees that `self` is in bounds of `slice` // which satisfies all the conditions for `add`. - let ptr = unsafe { slice.as_ptr().add(self.start) }; + let ptr = unsafe { + let this = ops::Range { ..self }; + assert_unsafe_precondition!( + "str::get_unchecked requires that the range is within the string slice", + (this: ops::Range, slice: *const [u8]) => + this.end >= this.start && this.end <= slice.len() + ); + slice.as_ptr().add(self.start) + }; let len = self.end - self.start; ptr::slice_from_raw_parts(ptr, len) as *const str } @@ -206,7 +215,15 @@ unsafe impl const SliceIndex for ops::Range { unsafe fn get_unchecked_mut(self, slice: *mut str) -> *mut Self::Output { let slice = slice as *mut [u8]; // SAFETY: see comments for `get_unchecked`. - let ptr = unsafe { slice.as_mut_ptr().add(self.start) }; + let ptr = unsafe { + let this = ops::Range { ..self }; + assert_unsafe_precondition!( + "str::get_unchecked_mut requires that the range is within the string slice", + (this: ops::Range, slice: *mut [u8]) => + this.end >= this.start && this.end <= slice.len() + ); + slice.as_mut_ptr().add(self.start) + }; let len = self.end - self.start; ptr::slice_from_raw_parts_mut(ptr, len) as *mut str } @@ -276,15 +293,13 @@ unsafe impl const SliceIndex for ops::RangeTo { } #[inline] unsafe fn get_unchecked(self, slice: *const str) -> *const Self::Output { - let slice = slice as *const [u8]; - let ptr = slice.as_ptr(); - ptr::slice_from_raw_parts(ptr, self.end) as *const str + // SAFETY: the caller has to uphold the safety contract for `get_unchecked`. + unsafe { (0..self.end).get_unchecked(slice) } } #[inline] unsafe fn get_unchecked_mut(self, slice: *mut str) -> *mut Self::Output { - let slice = slice as *mut [u8]; - let ptr = slice.as_mut_ptr(); - ptr::slice_from_raw_parts_mut(ptr, self.end) as *mut str + // SAFETY: the caller has to uphold the safety contract for `get_unchecked_mut`. + unsafe { (0..self.end).get_unchecked_mut(slice) } } #[inline] fn index(self, slice: &str) -> &Self::Output { @@ -347,20 +362,15 @@ unsafe impl const SliceIndex for ops::RangeFrom { } #[inline] unsafe fn get_unchecked(self, slice: *const str) -> *const Self::Output { - let slice = slice as *const [u8]; - // SAFETY: the caller guarantees that `self` is in bounds of `slice` - // which satisfies all the conditions for `add`. - let ptr = unsafe { slice.as_ptr().add(self.start) }; - let len = slice.len() - self.start; - ptr::slice_from_raw_parts(ptr, len) as *const str + let len = (slice as *const [u8]).len(); + // SAFETY: the caller has to uphold the safety contract for `get_unchecked`. + unsafe { (self.start..len).get_unchecked(slice) } } #[inline] unsafe fn get_unchecked_mut(self, slice: *mut str) -> *mut Self::Output { - let slice = slice as *mut [u8]; - // SAFETY: identical to `get_unchecked`. - let ptr = unsafe { slice.as_mut_ptr().add(self.start) }; - let len = slice.len() - self.start; - ptr::slice_from_raw_parts_mut(ptr, len) as *mut str + let len = (slice as *mut [u8]).len(); + // SAFETY: the caller has to uphold the safety contract for `get_unchecked_mut`. + unsafe { (self.start..len).get_unchecked_mut(slice) } } #[inline] fn index(self, slice: &str) -> &Self::Output { @@ -456,35 +466,29 @@ unsafe impl const SliceIndex for ops::RangeToInclusive { type Output = str; #[inline] fn get(self, slice: &str) -> Option<&Self::Output> { - if self.end == usize::MAX { None } else { (..self.end + 1).get(slice) } + (0..=self.end).get(slice) } #[inline] fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> { - if self.end == usize::MAX { None } else { (..self.end + 1).get_mut(slice) } + (0..=self.end).get_mut(slice) } #[inline] unsafe fn get_unchecked(self, slice: *const str) -> *const Self::Output { // SAFETY: the caller must uphold the safety contract for `get_unchecked`. - unsafe { (..self.end + 1).get_unchecked(slice) } + unsafe { (0..=self.end).get_unchecked(slice) } } #[inline] unsafe fn get_unchecked_mut(self, slice: *mut str) -> *mut Self::Output { // SAFETY: the caller must uphold the safety contract for `get_unchecked_mut`. - unsafe { (..self.end + 1).get_unchecked_mut(slice) } + unsafe { (0..=self.end).get_unchecked_mut(slice) } } #[inline] fn index(self, slice: &str) -> &Self::Output { - if self.end == usize::MAX { - str_index_overflow_fail(); - } - (..self.end + 1).index(slice) + (0..=self.end).index(slice) } #[inline] fn index_mut(self, slice: &mut str) -> &mut Self::Output { - if self.end == usize::MAX { - str_index_overflow_fail(); - } - (..self.end + 1).index_mut(slice) + (0..=self.end).index_mut(slice) } } From cd35794d5ea6688e77dd17e96556f5de0b410e8a Mon Sep 17 00:00:00 2001 From: Peter Jaszkowiak Date: Sat, 4 Mar 2023 15:11:24 -0700 Subject: [PATCH 2/2] Comment for why char boundaries aren't checked --- library/core/src/str/traits.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/core/src/str/traits.rs b/library/core/src/str/traits.rs index a7a2b03774a8f..b6d605c52d1ab 100644 --- a/library/core/src/str/traits.rs +++ b/library/core/src/str/traits.rs @@ -204,6 +204,12 @@ unsafe impl const SliceIndex for ops::Range { assert_unsafe_precondition!( "str::get_unchecked requires that the range is within the string slice", (this: ops::Range, slice: *const [u8]) => + // We'd like to check that the bounds are on char boundaries, + // but there's not really a way to do so without reading + // behind the pointer, which has aliasing implications. + // It's also not possible to move this check up to + // `str::get_unchecked` without adding a special function + // to `SliceIndex` just for this. this.end >= this.start && this.end <= slice.len() ); slice.as_ptr().add(self.start)