Skip to content

Commit

Permalink
Fix soundness issue for replace_range and range
Browse files Browse the repository at this point in the history
  • Loading branch information
dylni committed Jan 19, 2021
1 parent d98d2f5 commit b96063c
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 6 deletions.
13 changes: 10 additions & 3 deletions library/alloc/src/collections/btree/navigate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
13 changes: 10 additions & 3 deletions library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`]`>`.
Expand Down
50 changes: 50 additions & 0 deletions library/alloc/tests/string.rs
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -561,6 +565,52 @@ fn test_replace_range_unbounded() {
assert_eq!(s, "");
}

#[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();
Expand Down

0 comments on commit b96063c

Please sign in to comment.