Auto merge of #61224 - aloucks:drain_filter, r=Gankro

Prevent Vec::drain_filter from double dropping on panic

Fixes: #60977

The changes in this PR prevent leaking and double-panicking in addition to double-drop.

Tracking issue: #43244
This commit is contained in:
bors 2019-07-08 22:03:26 +00:00
commit 09ab31bc64
2 changed files with 169 additions and 9 deletions

View File

@ -944,6 +944,115 @@ fn drain_filter_complex() {
} }
} }
#[test]
#[cfg(not(miri))] // Miri does not support catching panics
fn drain_filter_consumed_panic() {
use std::rc::Rc;
use std::sync::Mutex;
struct Check {
index: usize,
drop_counts: Rc<Mutex<Vec<usize>>>,
};
impl Drop for Check {
fn drop(&mut self) {
self.drop_counts.lock().unwrap()[self.index] += 1;
println!("drop: {}", self.index);
}
}
let check_count = 10;
let drop_counts = Rc::new(Mutex::new(vec![0_usize; check_count]));
let mut data: Vec<Check> = (0..check_count)
.map(|index| Check { index, drop_counts: Rc::clone(&drop_counts) })
.collect();
let _ = std::panic::catch_unwind(move || {
let filter = |c: &mut Check| {
if c.index == 2 {
panic!("panic at index: {}", c.index);
}
// Verify that if the filter could panic again on another element
// that it would not cause a double panic and all elements of the
// vec would still be dropped exactly once.
if c.index == 4 {
panic!("panic at index: {}", c.index);
}
c.index < 6
};
let drain = data.drain_filter(filter);
// NOTE: The DrainFilter is explictly consumed
drain.for_each(drop);
});
let drop_counts = drop_counts.lock().unwrap();
assert_eq!(check_count, drop_counts.len());
for (index, count) in drop_counts.iter().cloned().enumerate() {
assert_eq!(1, count, "unexpected drop count at index: {} (count: {})", index, count);
}
}
#[test]
#[cfg(not(miri))] // Miri does not support catching panics
fn drain_filter_unconsumed_panic() {
use std::rc::Rc;
use std::sync::Mutex;
struct Check {
index: usize,
drop_counts: Rc<Mutex<Vec<usize>>>,
};
impl Drop for Check {
fn drop(&mut self) {
self.drop_counts.lock().unwrap()[self.index] += 1;
println!("drop: {}", self.index);
}
}
let check_count = 10;
let drop_counts = Rc::new(Mutex::new(vec![0_usize; check_count]));
let mut data: Vec<Check> = (0..check_count)
.map(|index| Check { index, drop_counts: Rc::clone(&drop_counts) })
.collect();
let _ = std::panic::catch_unwind(move || {
let filter = |c: &mut Check| {
if c.index == 2 {
panic!("panic at index: {}", c.index);
}
// Verify that if the filter could panic again on another element
// that it would not cause a double panic and all elements of the
// vec would still be dropped exactly once.
if c.index == 4 {
panic!("panic at index: {}", c.index);
}
c.index < 6
};
let _drain = data.drain_filter(filter);
// NOTE: The DrainFilter is dropped without being consumed
});
let drop_counts = drop_counts.lock().unwrap();
assert_eq!(check_count, drop_counts.len());
for (index, count) in drop_counts.iter().cloned().enumerate() {
assert_eq!(1, count, "unexpected drop count at index: {} (count: {})", index, count);
}
}
#[test]
fn drain_filter_unconsumed() {
let mut vec = vec![1, 2, 3, 4];
let drain = vec.drain_filter(|&mut x| x % 2 != 0);
drop(drain);
assert_eq!(vec, [2, 4]);
}
#[test] #[test]
fn test_reserve_exact() { fn test_reserve_exact() {
// This is all the same as test_reserve // This is all the same as test_reserve

View File

@ -2152,6 +2152,7 @@ impl<T> Vec<T> {
del: 0, del: 0,
old_len, old_len,
pred: filter, pred: filter,
panic_flag: false,
} }
} }
} }
@ -2779,10 +2780,20 @@ pub struct DrainFilter<'a, T, F>
where F: FnMut(&mut T) -> bool, where F: FnMut(&mut T) -> bool,
{ {
vec: &'a mut Vec<T>, vec: &'a mut Vec<T>,
/// The index of the item that will be inspected by the next call to `next`.
idx: usize, idx: usize,
/// The number of items that have been drained (removed) thus far.
del: usize, del: usize,
/// The original length of `vec` prior to draining.
old_len: usize, old_len: usize,
/// The filter test predicate.
pred: F, pred: F,
/// A flag that indicates a panic has occured in the filter test prodicate.
/// This is used as a hint in the drop implmentation to prevent consumption
/// of the remainder of the `DrainFilter`. Any unprocessed items will be
/// backshifted in the `vec`, but no further items will be dropped or
/// tested by the filter predicate.
panic_flag: bool,
} }
#[unstable(feature = "drain_filter", reason = "recently added", issue = "43244")] #[unstable(feature = "drain_filter", reason = "recently added", issue = "43244")]
@ -2793,20 +2804,23 @@ impl<T, F> Iterator for DrainFilter<'_, T, F>
fn next(&mut self) -> Option<T> { fn next(&mut self) -> Option<T> {
unsafe { unsafe {
while self.idx != self.old_len { while self.idx < self.old_len {
let i = self.idx; let i = self.idx;
self.idx += 1;
let v = slice::from_raw_parts_mut(self.vec.as_mut_ptr(), self.old_len); let v = slice::from_raw_parts_mut(self.vec.as_mut_ptr(), self.old_len);
if (self.pred)(&mut v[i]) { self.panic_flag = true;
let drained = (self.pred)(&mut v[i]);
self.panic_flag = false;
// Update the index *after* the predicate is called. If the index
// is updated prior and the predicate panics, the element at this
// index would be leaked.
self.idx += 1;
if drained {
self.del += 1; self.del += 1;
return Some(ptr::read(&v[i])); return Some(ptr::read(&v[i]));
} else if self.del > 0 { } else if self.del > 0 {
let del = self.del; let del = self.del;
let src: *const T = &v[i]; let src: *const T = &v[i];
let dst: *mut T = &mut v[i - del]; let dst: *mut T = &mut v[i - del];
// This is safe because self.vec has length 0
// thus its elements will not have Drop::drop
// called on them in the event of a panic.
ptr::copy_nonoverlapping(src, dst, 1); ptr::copy_nonoverlapping(src, dst, 1);
} }
} }
@ -2824,9 +2838,46 @@ impl<T, F> Drop for DrainFilter<'_, T, F>
where F: FnMut(&mut T) -> bool, where F: FnMut(&mut T) -> bool,
{ {
fn drop(&mut self) { fn drop(&mut self) {
self.for_each(drop); struct BackshiftOnDrop<'a, 'b, T, F>
unsafe { where
self.vec.set_len(self.old_len - self.del); F: FnMut(&mut T) -> bool,
{
drain: &'b mut DrainFilter<'a, T, F>,
}
impl<'a, 'b, T, F> Drop for BackshiftOnDrop<'a, 'b, T, F>
where
F: FnMut(&mut T) -> bool
{
fn drop(&mut self) {
unsafe {
if self.drain.idx < self.drain.old_len && self.drain.del > 0 {
// This is a pretty messed up state, and there isn't really an
// obviously right thing to do. We don't want to keep trying
// to execute `pred`, so we just backshift all the unprocessed
// elements and tell the vec that they still exist. The backshift
// is required to prevent a double-drop of the last successfully
// drained item prior to a panic in the predicate.
let ptr = self.drain.vec.as_mut_ptr();
let src = ptr.add(self.drain.idx);
let dst = src.sub(self.drain.del);
let tail_len = self.drain.old_len - self.drain.idx;
src.copy_to(dst, tail_len);
}
self.drain.vec.set_len(self.drain.old_len - self.drain.del);
}
}
}
let backshift = BackshiftOnDrop {
drain: self
};
// Attempt to consume any remaining elements if the filter predicate
// has not yet panicked. We'll backshift any remaining elements
// whether we've already panicked or if the consumption here panics.
if !backshift.drain.panic_flag {
backshift.drain.for_each(drop);
} }
} }
} }