Fix soundness issue for `replace_range` and `range`

This commit is contained in:
dylni 2021-01-18 22:14:38 -05:00
parent d98d2f57d9
commit b96063cf47
3 changed files with 70 additions and 6 deletions

View File

@ -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;

View File

@ -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`]`>`.

View File

@ -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
@ -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();