Fix soundness issue for `replace_range` and `range`

3 files changed, 70 insertions, 6 deletions

diff --git a/library/alloc/src/collections/btree/navigate.rs b/library/alloc/src/collections/btree/navigate.rs
index 45a8e0103a4..17b4689e4a0 100644
--- a/library/alloc/src/collections/btree/navigate.rs
+++ b/library/alloc/src/collections/btree/navigate.rs
@@ -25,7 +25,12 @@ where
     K: Borrow<Q>,
     R: RangeBounds<Q>,
 {
-    match (range.start_bound(), range.end_bound()) {
+    // WARNING: Inlining these variables would be unsound (#81138)
+    // We assume the bounds reported by `range` remain the same, but
+    // an adversarial implementation could change between calls
+    let start = range.start_bound();
+    let end = range.end_bound();
+    match (start, end) {
         (Excluded(s), Excluded(e)) if s == e => {
             panic!("range start and end are equal and excluded in BTreeMap")
         }
@@ -41,7 +46,8 @@ where
     let mut max_found = false;
 
     loop {
-        let front = match (min_found, range.start_bound()) {
+        // Using `range` again would be unsound (#81138)
+        let front = match (min_found, start) {
             (false, Included(key)) => match min_node.search_node(key) {
                 SearchResult::Found(kv) => {
                     min_found = true;
@@ -61,7 +67,8 @@ where
             (_, Unbounded) => min_node.first_edge(),
         };
 
-        let back = match (max_found, range.end_bound()) {
+        // Using `range` again would be unsound (#81138)
+        let back = match (max_found, end) {
             (false, Included(key)) => match max_node.search_node(key) {
                 SearchResult::Found(kv) => {
                     max_found = true;
diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs
index 91988928593..6f2a497598d 100644
--- a/library/alloc/src/string.rs
+++ b/library/alloc/src/string.rs
@@ -1553,18 +1553,25 @@ impl String {
         // Replace_range does not have the memory safety issues of a vector Splice.
         // of the vector version. The data is just plain bytes.
 
-        match range.start_bound() {
+        // WARNING: Inlining this variable would be unsound (#81138)
+        let start = range.start_bound();
+        match start {
             Included(&n) => assert!(self.is_char_boundary(n)),
             Excluded(&n) => assert!(self.is_char_boundary(n + 1)),
             Unbounded => {}
         };
-        match range.end_bound() {
+        // WARNING: Inlining this variable would be unsound (#81138)
+        let end = range.end_bound();
+        match end {
             Included(&n) => assert!(self.is_char_boundary(n + 1)),
             Excluded(&n) => assert!(self.is_char_boundary(n)),
             Unbounded => {}
         };
 
-        unsafe { self.as_mut_vec() }.splice(range, replace_with.bytes());
+        // Using `range` again would be unsound (#81138)
+        // We assume the bounds reported by `range` remain the same, but
+        // an adversarial implementation could change between calls
+        unsafe { self.as_mut_vec() }.splice((start, end), replace_with.bytes());
     }
 
     /// Converts this `String` into a [`Box`]`<`[`str`]`>`.
diff --git a/library/alloc/tests/string.rs b/library/alloc/tests/string.rs
index b28694186b6..f3d74e0514d 100644
--- a/library/alloc/tests/string.rs
+++ b/library/alloc/tests/string.rs
@@ -1,7 +1,11 @@
 use std::borrow::Cow;
+use std::cell::Cell;
 use std::collections::TryReserveError::*;
+use std::ops::Bound;
 use std::ops::Bound::*;
+use std::ops::RangeBounds;
 use std::panic;
+use std::str;
 
 pub trait IntoCow<'a, B: ?Sized>
 where
@@ -562,6 +566,52 @@ fn test_replace_range_unbounded() {
 }
 
 #[test]
+fn test_replace_range_evil_start_bound() {
+    struct EvilRange(Cell<bool>);
+
+    impl RangeBounds<usize> for EvilRange {
+        fn start_bound(&self) -> Bound<&usize> {
+            Bound::Included(if self.0.get() {
+                &1
+            } else {
+                self.0.set(true);
+                &0
+            })
+        }
+        fn end_bound(&self) -> Bound<&usize> {
+            Bound::Unbounded
+        }
+    }
+
+    let mut s = String::from("🦀");
+    s.replace_range(EvilRange(Cell::new(false)), "");
+    assert_eq!(Ok(""), str::from_utf8(s.as_bytes()));
+}
+
+#[test]
+fn test_replace_range_evil_end_bound() {
+    struct EvilRange(Cell<bool>);
+
+    impl RangeBounds<usize> for EvilRange {
+        fn start_bound(&self) -> Bound<&usize> {
+            Bound::Included(&0)
+        }
+        fn end_bound(&self) -> Bound<&usize> {
+            Bound::Excluded(if self.0.get() {
+                &3
+            } else {
+                self.0.set(true);
+                &4
+            })
+        }
+    }
+
+    let mut s = String::from("🦀");
+    s.replace_range(EvilRange(Cell::new(false)), "");
+    assert_eq!(Ok(""), str::from_utf8(s.as_bytes()));
+}
+
+#[test]
 fn test_extend_ref() {
     let mut a = "foo".to_string();
     a.extend(&['b', 'a', 'r']);