From bb70e52f5f5ee25012193f9f4f4372702b97b20e Mon Sep 17 00:00:00 2001 From: dylni <46035563+dylni@users.noreply.github.com> Date: Wed, 5 Aug 2020 22:32:45 -0400 Subject: [PATCH 1/5] Add `slice::check_range` --- library/core/src/slice/mod.rs | 93 ++++++++++++++++++++++++++++++++--- 1 file changed, 87 insertions(+), 6 deletions(-) diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 93608a1ce4864..d5e07629a52cb 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -30,7 +30,7 @@ use crate::intrinsics::{assume, exact_div, is_aligned_and_not_null, unchecked_su use crate::iter::*; use crate::marker::{self, Copy, Send, Sized, Sync}; use crate::mem; -use crate::ops::{self, FnMut, Range}; +use crate::ops::{self, Bound, FnMut, Range, RangeBounds}; use crate::option::Option; use crate::option::Option::{None, Some}; use crate::ptr::{self, NonNull}; @@ -350,6 +350,80 @@ impl [T] { unsafe { &mut *index.get_unchecked_mut(self) } } + /// Converts a range over this slice to [`Range`]. + /// + /// The returned range is safe to pass to [`get_unchecked`] and [`get_unchecked_mut`]. + /// + /// [`get_unchecked`]: #method.get_unchecked + /// [`get_unchecked_mut`]: #method.get_unchecked_mut + /// [`Range`]: ../ops/struct.Range.html + /// + /// # Panics + /// + /// Panics if the range is out of bounds. + /// + /// # Examples + /// + /// ``` + /// #![feature(slice_check_range)] + /// + /// let v = [10, 40, 30]; + /// assert_eq!(1..2, v.check_range(1..2)); + /// assert_eq!(0..2, v.check_range(..2)); + /// assert_eq!(1..3, v.check_range(1..)); + /// ``` + /// + /// Panics when [`Index::index`] would panic: + /// + /// ```should_panic + /// #![feature(slice_check_range)] + /// + /// [10, 40, 30].check_range(2..1); + /// ``` + /// + /// ```should_panic + /// #![feature(slice_check_range)] + /// + /// [10, 40, 30].check_range(1..4); + /// ``` + /// + /// ```should_panic + /// #![feature(slice_check_range)] + /// + /// [10, 40, 30].check_range(1..=usize::MAX); + /// ``` + /// + /// [`Index::index`]: ../ops/trait.Index.html#tymethod.index + #[track_caller] + #[unstable(feature = "slice_check_range", issue = "none")] + pub fn check_range>(&self, range: R) -> Range { + let start = match range.start_bound() { + Bound::Included(&start) => start, + Bound::Excluded(start) => { + start.checked_add(1).unwrap_or_else(|| slice_start_index_overflow_fail()) + } + Bound::Unbounded => 0, + }; + + let len = self.len(); + let end = match range.end_bound() { + Bound::Included(end) => { + end.checked_add(1).unwrap_or_else(|| slice_end_index_overflow_fail()) + } + Bound::Excluded(&end) => end, + Bound::Unbounded => len, + }; + + if start > end { + slice_index_order_fail(start, end); + } + if end > len { + slice_end_index_len_fail(end, len); + } + + Range { start, end } + } + /// Returns a raw pointer to the slice's buffer. /// /// The caller must ensure that the slice outlives the pointer this @@ -2445,13 +2519,13 @@ impl [T] { let src_start = match src.start_bound() { ops::Bound::Included(&n) => n, ops::Bound::Excluded(&n) => { - n.checked_add(1).unwrap_or_else(|| slice_index_overflow_fail()) + n.checked_add(1).unwrap_or_else(|| slice_start_index_overflow_fail()) } ops::Bound::Unbounded => 0, }; let src_end = match src.end_bound() { ops::Bound::Included(&n) => { - n.checked_add(1).unwrap_or_else(|| slice_index_overflow_fail()) + n.checked_add(1).unwrap_or_else(|| slice_end_index_overflow_fail()) } ops::Bound::Excluded(&n) => n, ops::Bound::Unbounded => self.len(), @@ -3034,7 +3108,14 @@ fn slice_index_order_fail(index: usize, end: usize) -> ! { #[inline(never)] #[cold] #[track_caller] -fn slice_index_overflow_fail() -> ! { +fn slice_start_index_overflow_fail() -> ! { + panic!("attempted to index slice from after maximum usize"); +} + +#[inline(never)] +#[cold] +#[track_caller] +fn slice_end_index_overflow_fail() -> ! { panic!("attempted to index slice up to maximum usize"); } @@ -3370,7 +3451,7 @@ unsafe impl SliceIndex<[T]> for ops::RangeInclusive { #[inline] fn index(self, slice: &[T]) -> &[T] { if *self.end() == usize::MAX { - slice_index_overflow_fail(); + slice_end_index_overflow_fail(); } (*self.start()..self.end() + 1).index(slice) } @@ -3378,7 +3459,7 @@ unsafe impl SliceIndex<[T]> for ops::RangeInclusive { #[inline] fn index_mut(self, slice: &mut [T]) -> &mut [T] { if *self.end() == usize::MAX { - slice_index_overflow_fail(); + slice_end_index_overflow_fail(); } (*self.start()..self.end() + 1).index_mut(slice) } From 202b242d87c30c2fe1475342f9b93a8dfc65cd17 Mon Sep 17 00:00:00 2001 From: dylni <46035563+dylni@users.noreply.github.com> Date: Wed, 5 Aug 2020 23:16:18 -0400 Subject: [PATCH 2/5] Fix links --- library/core/src/slice/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index d5e07629a52cb..8c6c0111c33cc 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -356,7 +356,7 @@ impl [T] { /// /// [`get_unchecked`]: #method.get_unchecked /// [`get_unchecked_mut`]: #method.get_unchecked_mut - /// [`Range`]: ../ops/struct.Range.html + /// [`Range`]: ops/struct.Range.html /// /// # Panics /// @@ -393,7 +393,7 @@ impl [T] { /// [10, 40, 30].check_range(1..=usize::MAX); /// ``` /// - /// [`Index::index`]: ../ops/trait.Index.html#tymethod.index + /// [`Index::index`]: ops/trait.Index.html#tymethod.index #[track_caller] #[unstable(feature = "slice_check_range", issue = "none")] pub fn check_range>(&self, range: R) -> Range { From ed02b90e9b76b9ce2e8e99a99dbadd96ab4dfb42 Mon Sep 17 00:00:00 2001 From: dylni <46035563+dylni@users.noreply.github.com> Date: Thu, 6 Aug 2020 14:14:29 -0400 Subject: [PATCH 3/5] Fix links again --- library/core/src/slice/mod.rs | 3 +-- src/tools/linkchecker/main.rs | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 8c6c0111c33cc..dafb3e44d7576 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -356,7 +356,6 @@ impl [T] { /// /// [`get_unchecked`]: #method.get_unchecked /// [`get_unchecked_mut`]: #method.get_unchecked_mut - /// [`Range`]: ops/struct.Range.html /// /// # Panics /// @@ -393,7 +392,7 @@ impl [T] { /// [10, 40, 30].check_range(1..=usize::MAX); /// ``` /// - /// [`Index::index`]: ops/trait.Index.html#tymethod.index + /// [`Index::index`]: ops::Index::index #[track_caller] #[unstable(feature = "slice_check_range", issue = "none")] pub fn check_range>(&self, range: R) -> Range { diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index 194318d7a59b5..20a56ae5b9dca 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -39,6 +39,7 @@ const LINKCHECK_EXCEPTIONS: &[(&str, &[&str])] = &[ "#method.sort_by_key", "#method.make_ascii_uppercase", "#method.make_ascii_lowercase", + "#method.get_unchecked_mut", ], ), // These try to link to std::collections, but are defined in alloc From d04e6b8de5fe6bbf203c534c35e6f55e8960ab46 Mon Sep 17 00:00:00 2001 From: dylni <46035563+dylni@users.noreply.github.com> Date: Sun, 16 Aug 2020 21:47:12 -0400 Subject: [PATCH 4/5] Replace ad hoc implementations with `slice::check_range` --- library/alloc/src/collections/vec_deque.rs | 37 +++++++--------------- library/alloc/src/lib.rs | 1 + library/alloc/src/string.rs | 20 ++++-------- library/alloc/src/vec.rs | 33 ++----------------- library/core/src/slice/mod.rs | 19 ++--------- library/core/tests/slice.rs | 4 +-- 6 files changed, 24 insertions(+), 90 deletions(-) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index d3c6d493d6d8d..54cf548fac521 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -14,8 +14,7 @@ use core::fmt; use core::hash::{Hash, Hasher}; use core::iter::{once, repeat_with, FromIterator, FusedIterator}; use core::mem::{self, replace, ManuallyDrop}; -use core::ops::Bound::{Excluded, Included, Unbounded}; -use core::ops::{Index, IndexMut, RangeBounds, Try}; +use core::ops::{Index, IndexMut, Range, RangeBounds, Try}; use core::ptr::{self, NonNull}; use core::slice; @@ -1083,24 +1082,16 @@ impl VecDeque { self.tail == self.head } - fn range_start_end(&self, range: R) -> (usize, usize) + fn range_tail_head(&self, range: R) -> (usize, usize) where R: RangeBounds, { - let len = self.len(); - let start = match range.start_bound() { - Included(&n) => n, - Excluded(&n) => n + 1, - Unbounded => 0, - }; - let end = match range.end_bound() { - Included(&n) => n + 1, - Excluded(&n) => n, - Unbounded => len, - }; - assert!(start <= end, "lower bound was too large"); - assert!(end <= len, "upper bound was too large"); - (start, end) + // SAFETY: This buffer is only used to check the range. + let buffer = unsafe { slice::from_raw_parts(self.ptr(), self.len()) }; + let Range { start, end } = buffer.check_range(range); + let tail = self.wrap_add(self.tail, start); + let head = self.wrap_add(self.tail, end); + (tail, head) } /// Creates an iterator that covers the specified range in the `VecDeque`. @@ -1131,9 +1122,7 @@ impl VecDeque { where R: RangeBounds, { - let (start, end) = self.range_start_end(range); - let tail = self.wrap_add(self.tail, start); - let head = self.wrap_add(self.tail, end); + let (tail, head) = self.range_tail_head(range); Iter { tail, head, @@ -1174,9 +1163,7 @@ impl VecDeque { where R: RangeBounds, { - let (start, end) = self.range_start_end(range); - let tail = self.wrap_add(self.tail, start); - let head = self.wrap_add(self.tail, end); + let (tail, head) = self.range_tail_head(range); IterMut { tail, head, @@ -1230,7 +1217,7 @@ impl VecDeque { // When finished, the remaining data will be copied back to cover the hole, // and the head/tail values will be restored correctly. // - let (start, end) = self.range_start_end(range); + let (drain_tail, drain_head) = self.range_tail_head(range); // The deque's elements are parted into three segments: // * self.tail -> drain_tail @@ -1248,8 +1235,6 @@ impl VecDeque { // T t h H // [. . . o o x x o o . . .] // - let drain_tail = self.wrap_add(self.tail, start); - let drain_head = self.wrap_add(self.tail, end); let head = self.head; // "forget" about the values after the start of the drain until after diff --git a/library/alloc/src/lib.rs b/library/alloc/src/lib.rs index 9ac23886d4e15..514b0bf918ac5 100644 --- a/library/alloc/src/lib.rs +++ b/library/alloc/src/lib.rs @@ -114,6 +114,7 @@ #![feature(rustc_attrs)] #![feature(receiver_trait)] #![feature(min_specialization)] +#![feature(slice_check_range)] #![feature(slice_ptr_get)] #![feature(slice_ptr_len)] #![feature(staged_api)] diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index d7d7b6bd157bc..b3ac4397f17ac 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -47,7 +47,7 @@ use core::fmt; use core::hash; use core::iter::{FromIterator, FusedIterator}; use core::ops::Bound::{Excluded, Included, Unbounded}; -use core::ops::{self, Add, AddAssign, Index, IndexMut, RangeBounds}; +use core::ops::{self, Add, AddAssign, Index, IndexMut, Range, RangeBounds}; use core::ptr; use core::str::{lossy, pattern::Pattern}; @@ -1506,23 +1506,15 @@ impl String { // of the vector version. The data is just plain bytes. // Because the range removal happens in Drop, if the Drain iterator is leaked, // the removal will not happen. - let len = self.len(); - let start = match range.start_bound() { - Included(&n) => n, - Excluded(&n) => n + 1, - Unbounded => 0, - }; - let end = match range.end_bound() { - Included(&n) => n + 1, - Excluded(&n) => n, - Unbounded => len, - }; + let Range { start, end } = self.as_bytes().check_range(range); + assert!(self.is_char_boundary(start)); + assert!(self.is_char_boundary(end)); // Take out two simultaneous borrows. The &mut String won't be accessed // until iteration is over, in Drop. let self_ptr = self as *mut _; - // slicing does the appropriate bounds checks - let chars_iter = self[start..end].chars(); + // SAFETY: `check_range` and `is_char_boundary` do the appropriate bounds checks. + let chars_iter = unsafe { self.get_unchecked(start..end) }.chars(); Drain { start, end, iter: chars_iter, string: self_ptr } } diff --git a/library/alloc/src/vec.rs b/library/alloc/src/vec.rs index 786d1b6ba82f2..a29985256aae9 100644 --- a/library/alloc/src/vec.rs +++ b/library/alloc/src/vec.rs @@ -66,8 +66,7 @@ use core::intrinsics::{arith_offset, assume}; use core::iter::{FromIterator, FusedIterator, TrustedLen}; use core::marker::PhantomData; use core::mem::{self, ManuallyDrop, MaybeUninit}; -use core::ops::Bound::{Excluded, Included, Unbounded}; -use core::ops::{self, Index, IndexMut, RangeBounds}; +use core::ops::{self, Index, IndexMut, Range, RangeBounds}; use core::ptr::{self, NonNull}; use core::slice::{self, SliceIndex}; @@ -1311,35 +1310,7 @@ impl Vec { // the hole, and the vector length is restored to the new length. // let len = self.len(); - let start = match range.start_bound() { - Included(&n) => n, - Excluded(&n) => n + 1, - Unbounded => 0, - }; - let end = match range.end_bound() { - Included(&n) => n + 1, - Excluded(&n) => n, - Unbounded => len, - }; - - #[cold] - #[inline(never)] - fn start_assert_failed(start: usize, end: usize) -> ! { - panic!("start drain index (is {}) should be <= end drain index (is {})", start, end); - } - - #[cold] - #[inline(never)] - fn end_assert_failed(end: usize, len: usize) -> ! { - panic!("end drain index (is {}) should be <= len (is {})", end, len); - } - - if start > end { - start_assert_failed(start, end); - } - if end > len { - end_assert_failed(end, len); - } + let Range { start, end } = self.check_range(range); unsafe { // set self.vec length's to start, to be safe in case Drain is leaked diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index dafb3e44d7576..066368d5de56a 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -2511,26 +2511,11 @@ impl [T] { /// ``` #[stable(feature = "copy_within", since = "1.37.0")] #[track_caller] - pub fn copy_within>(&mut self, src: R, dest: usize) + pub fn copy_within>(&mut self, src: R, dest: usize) where T: Copy, { - let src_start = match src.start_bound() { - ops::Bound::Included(&n) => n, - ops::Bound::Excluded(&n) => { - n.checked_add(1).unwrap_or_else(|| slice_start_index_overflow_fail()) - } - ops::Bound::Unbounded => 0, - }; - let src_end = match src.end_bound() { - ops::Bound::Included(&n) => { - n.checked_add(1).unwrap_or_else(|| slice_end_index_overflow_fail()) - } - ops::Bound::Excluded(&n) => n, - ops::Bound::Unbounded => self.len(), - }; - assert!(src_start <= src_end, "src end is before src start"); - assert!(src_end <= self.len(), "src is out of bounds"); + let Range { start: src_start, end: src_end } = self.check_range(src); let count = src_end - src_start; assert!(dest <= self.len() - count, "dest is out of bounds"); unsafe { diff --git a/library/core/tests/slice.rs b/library/core/tests/slice.rs index 42b483f33ba44..abe321c9daae4 100644 --- a/library/core/tests/slice.rs +++ b/library/core/tests/slice.rs @@ -1797,7 +1797,7 @@ fn test_copy_within() { } #[test] -#[should_panic(expected = "src is out of bounds")] +#[should_panic(expected = "range end index 14 out of range for slice of length 13")] fn test_copy_within_panics_src_too_long() { let mut bytes = *b"Hello, World!"; // The length is only 13, so 14 is out of bounds. @@ -1812,7 +1812,7 @@ fn test_copy_within_panics_dest_too_long() { bytes.copy_within(0..4, 10); } #[test] -#[should_panic(expected = "src end is before src start")] +#[should_panic(expected = "slice index starts at 2 but ends at 1")] fn test_copy_within_panics_src_inverted() { let mut bytes = *b"Hello, World!"; // 2 is greater than 1, so this range is invalid. From d9e877fb98212a47dd425e145b8b3e4283e6b487 Mon Sep 17 00:00:00 2001 From: dylni <46035563+dylni@users.noreply.github.com> Date: Mon, 24 Aug 2020 10:53:25 -0400 Subject: [PATCH 5/5] Add more information to safety comment --- library/alloc/src/collections/vec_deque.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/collections/vec_deque.rs b/library/alloc/src/collections/vec_deque.rs index 54cf548fac521..04014c8eb5a95 100644 --- a/library/alloc/src/collections/vec_deque.rs +++ b/library/alloc/src/collections/vec_deque.rs @@ -1086,7 +1086,9 @@ impl VecDeque { where R: RangeBounds, { - // SAFETY: This buffer is only used to check the range. + // SAFETY: This buffer is only used to check the range. It might be partially + // uninitialized, but `check_range` needs a contiguous slice. + // https://github.com/rust-lang/rust/pull/75207#discussion_r471193682 let buffer = unsafe { slice::from_raw_parts(self.ptr(), self.len()) }; let Range { start, end } = buffer.check_range(range); let tail = self.wrap_add(self.tail, start);