From a859ca5c87ddfaa635fb4cacf8a41e04fd9b02e8 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 14:51:37 +0100 Subject: [PATCH 01/11] Fix `binary_heap::DrainSorted` drop leak on panics --- src/liballoc/collections/binary_heap.rs | 16 ++++++++++-- src/liballoc/tests/binary_heap.rs | 33 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/liballoc/collections/binary_heap.rs b/src/liballoc/collections/binary_heap.rs index c527b378f7465..f38fe997b732c 100644 --- a/src/liballoc/collections/binary_heap.rs +++ b/src/liballoc/collections/binary_heap.rs @@ -147,7 +147,7 @@ use core::fmt; use core::iter::{FromIterator, FusedIterator, TrustedLen}; -use core::mem::{size_of, swap, ManuallyDrop}; +use core::mem::{self, size_of, swap, ManuallyDrop}; use core::ops::{Deref, DerefMut}; use core::ptr; @@ -1239,7 +1239,19 @@ pub struct DrainSorted<'a, T: Ord> { impl<'a, T: Ord> Drop for DrainSorted<'a, T> { /// Removes heap elements in heap order. fn drop(&mut self) { - while let Some(_) = self.inner.pop() {} + struct DropGuard<'r, 'a, T: Ord>(&'r mut DrainSorted<'a, T>); + + impl<'r, 'a, T: Ord> Drop for DropGuard<'r, 'a, T> { + fn drop(&mut self) { + while let Some(_) = self.0.inner.pop() {} + } + } + + while let Some(item) = self.inner.pop() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } } } diff --git a/src/liballoc/tests/binary_heap.rs b/src/liballoc/tests/binary_heap.rs index f49ca7139212f..be5516f54f37b 100644 --- a/src/liballoc/tests/binary_heap.rs +++ b/src/liballoc/tests/binary_heap.rs @@ -1,6 +1,8 @@ use std::collections::binary_heap::{Drain, PeekMut}; use std::collections::BinaryHeap; use std::iter::TrustedLen; +use std::panic::{catch_unwind, AssertUnwindSafe}; +use std::sync::atomic::{AtomicU32, Ordering}; #[test] fn test_iterator() { @@ -275,6 +277,37 @@ fn test_drain_sorted() { assert!(q.is_empty()); } +#[test] +fn test_drain_sorted_leak() { + static DROPS: AtomicU32 = AtomicU32::new(0); + + #[derive(Clone, PartialEq, Eq, PartialOrd, Ord)] + struct D(u32, bool); + + impl Drop for D { + fn drop(&mut self) { + DROPS.fetch_add(1, Ordering::SeqCst); + + if self.1 { + panic!("panic in `drop`"); + } + } + } + + let mut q = BinaryHeap::from(vec![ + D(0, false), + D(1, false), + D(2, false), + D(3, true), + D(4, false), + D(5, false), + ]); + + catch_unwind(AssertUnwindSafe(|| drop(q.drain_sorted()))).ok(); + + assert_eq!(DROPS.load(Ordering::SeqCst), 6); +} + #[test] fn test_extend_ref() { let mut a = BinaryHeap::new(); From 3e5eb2634cbb356b626e028a4be1305d4a44a023 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 16:41:19 +0100 Subject: [PATCH 02/11] Fix `VecDeque::truncate` leak on drop panic --- src/liballoc/collections/vec_deque.rs | 17 ++++++++++++- src/liballoc/tests/vec_deque.rs | 35 ++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/liballoc/collections/vec_deque.rs b/src/liballoc/collections/vec_deque.rs index 2cc450bb68a20..7307aad8a9ed6 100644 --- a/src/liballoc/collections/vec_deque.rs +++ b/src/liballoc/collections/vec_deque.rs @@ -866,6 +866,18 @@ impl VecDeque { /// ``` #[stable(feature = "deque_extras", since = "1.16.0")] pub fn truncate(&mut self, len: usize) { + /// Runs the destructor for all items in the slice when it gets dropped (normally or + /// during unwinding). + struct Dropper<'a, T>(&'a mut [T]); + + impl<'a, T> Drop for Dropper<'a, T> { + fn drop(&mut self) { + unsafe { + ptr::drop_in_place(self.0); + } + } + } + // Safe because: // // * Any slice passed to `drop_in_place` is valid; the second case has @@ -888,8 +900,11 @@ impl VecDeque { let drop_back = back as *mut _; let drop_front = front.get_unchecked_mut(len..) as *mut _; self.head = self.wrap_sub(self.head, num_dropped); + + // Make sure the second half is dropped even when a destructor + // in the first one panics. + let _back_dropper = Dropper(&mut *drop_back); ptr::drop_in_place(drop_front); - ptr::drop_in_place(drop_back); } } } diff --git a/src/liballoc/tests/vec_deque.rs b/src/liballoc/tests/vec_deque.rs index 1ab3694a3ca61..2dc50d0c70e23 100644 --- a/src/liballoc/tests/vec_deque.rs +++ b/src/liballoc/tests/vec_deque.rs @@ -2,7 +2,7 @@ use std::collections::TryReserveError::*; use std::collections::{vec_deque::Drain, VecDeque}; use std::fmt::Debug; use std::mem::size_of; -use std::panic::catch_unwind; +use std::panic::{AssertUnwindSafe, catch_unwind}; use std::{isize, usize}; use crate::hash; @@ -1573,3 +1573,36 @@ fn test_try_rfold_moves_iter() { assert_eq!(iter.try_rfold(0_i8, |acc, &x| acc.checked_add(x)), None); assert_eq!(iter.next_back(), Some(&70)); } + +#[test] +fn truncate_leak() { + static mut DROPS: i32 = 0; + + struct D(bool); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + + if self.0 { + panic!("panic in `drop`"); + } + } + } + + let mut q = VecDeque::new(); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_front(D(true)); + q.push_front(D(false)); + q.push_front(D(false)); + + catch_unwind(AssertUnwindSafe(|| q.truncate(1))).ok(); + + assert_eq!(unsafe { DROPS }, 7); +} From 5d04790dd2e73f3faf08d528e3675e131585ec01 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 17:54:26 +0100 Subject: [PATCH 03/11] Avoid leak in `vec::Drain` when item drop panics --- src/liballoc/tests/vec.rs | 39 +++++++++++++++++++++++++++++++++++ src/liballoc/vec.rs | 43 +++++++++++++++++++++++++++------------ 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 2a9bfefc713e7..80acba0a3a162 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::collections::TryReserveError::*; use std::mem::size_of; +use std::panic::{catch_unwind, AssertUnwindSafe}; use std::vec::{Drain, IntoIter}; use std::{isize, usize}; @@ -585,6 +586,44 @@ fn test_drain_inclusive_out_of_bounds() { v.drain(5..=5); } +#[test] +fn test_drain_leak() { + static mut DROPS: i32 = 0; + + #[derive(Debug, PartialEq)] + struct D(u32, bool); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + + if self.1 { + panic!("panic in `drop`"); + } + } + } + + let mut v = vec![ + D(0, false), + D(1, false), + D(2, false), + D(3, false), + D(4, true), + D(5, false), + D(6, false), + ]; + + catch_unwind(AssertUnwindSafe(|| { + v.drain(2..=5); + })) + .ok(); + + assert_eq!(unsafe { DROPS }, 4); + assert_eq!(v, vec![D(0, false), D(1, false), D(6, false),]); +} + #[test] fn test_splice() { let mut v = vec![1, 2, 3, 4, 5]; diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index e9cbf627846b5..ba71e42090cbb 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -2700,23 +2700,40 @@ impl DoubleEndedIterator for Drain<'_, T> { #[stable(feature = "drain", since = "1.6.0")] impl Drop for Drain<'_, T> { fn drop(&mut self) { - // exhaust self first - self.for_each(drop); + /// Continues dropping the remaining elements when a destructor unwinds. + struct DropGuard<'r, 'a, T>(&'r mut Drain<'a, T>); - if self.tail_len > 0 { - unsafe { - let source_vec = self.vec.as_mut(); - // memmove back untouched tail, update to new length - let start = source_vec.len(); - let tail = self.tail_start; - if tail != start { - let src = source_vec.as_ptr().add(tail); - let dst = source_vec.as_mut_ptr().add(start); - ptr::copy(src, dst, self.tail_len); + impl<'r, 'a, T> Drop for DropGuard<'r, 'a, T> { + fn drop(&mut self) { + // Continue the same loop we do below. This only runs when a destructor has + // panicked. If another one panics this will abort. + self.0.for_each(drop); + + if self.0.tail_len > 0 { + unsafe { + let source_vec = self.0.vec.as_mut(); + // memmove back untouched tail, update to new length + let start = source_vec.len(); + let tail = self.0.tail_start; + if tail != start { + let src = source_vec.as_ptr().add(tail); + let dst = source_vec.as_mut_ptr().add(start); + ptr::copy(src, dst, self.0.tail_len); + } + source_vec.set_len(start + self.0.tail_len); + } } - source_vec.set_len(start + self.tail_len); } } + + // exhaust self first + while let Some(item) = self.next() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } + + DropGuard(self); } } From dc492452dae29d75b14afe3559f5fb59be7f2d3a Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 18:36:35 +0100 Subject: [PATCH 04/11] Fix leak in btree_map::IntoIter when drop panics --- src/liballoc/collections/btree/map.rs | 17 +++++++++++++++- src/liballoc/tests/btree/map.rs | 28 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 302c2bcd5e4a3..71ddfc4ef63b6 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -1381,7 +1381,22 @@ impl IntoIterator for BTreeMap { #[stable(feature = "btree_drop", since = "1.7.0")] impl Drop for IntoIter { fn drop(&mut self) { - self.for_each(drop); + struct DropGuard<'a, K, V>(&'a mut IntoIter); + + impl<'a, K, V> Drop for DropGuard<'a, K, V> { + fn drop(&mut self) { + // Continue the same loop we perform below. This only runs when unwinding, so we + // don't have to care about panics this time (they'll abort). + while let Some(_) = self.0.next() {} + } + } + + while let Some(pair) = self.next() { + let guard = DropGuard(self); + drop(pair); + mem::forget(guard); + } + unsafe { let leaf_node = ptr::read(&self.front).into_node(); if leaf_node.is_shared_root() { diff --git a/src/liballoc/tests/btree/map.rs b/src/liballoc/tests/btree/map.rs index f5be72c39b20c..0729f341d8609 100644 --- a/src/liballoc/tests/btree/map.rs +++ b/src/liballoc/tests/btree/map.rs @@ -5,7 +5,9 @@ use std::fmt::Debug; use std::iter::FromIterator; use std::ops::Bound::{self, Excluded, Included, Unbounded}; use std::ops::RangeBounds; +use std::panic::catch_unwind; use std::rc::Rc; +use std::sync::atomic::{AtomicU32, Ordering}; use super::DeterministicRng; @@ -980,3 +982,29 @@ fn test_split_off_large_random_sorted() { assert!(map.into_iter().eq(data.clone().into_iter().filter(|x| x.0 < key))); assert!(right.into_iter().eq(data.into_iter().filter(|x| x.0 >= key))); } + +#[test] +fn test_into_iter_drop_leak() { + static DROPS: AtomicU32 = AtomicU32::new(0); + + struct D; + + impl Drop for D { + fn drop(&mut self) { + if DROPS.fetch_add(1, Ordering::SeqCst) == 3 { + panic!("panic in `drop`"); + } + } + } + + let mut map = BTreeMap::new(); + map.insert("a", D); + map.insert("b", D); + map.insert("c", D); + map.insert("d", D); + map.insert("e", D); + + catch_unwind(move || drop(map.into_iter())).ok(); + + assert_eq!(DROPS.load(Ordering::SeqCst), 5); +} From b04ca13873d5ef5e5b18195aaf8925562302e8e0 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 19:32:16 +0100 Subject: [PATCH 05/11] Fix leak in VecDeque::drain when drop panics --- src/liballoc/collections/vec_deque.rs | 82 ++++++++++++++++----------- src/liballoc/tests/vec_deque.rs | 38 +++++++++++++ 2 files changed, 86 insertions(+), 34 deletions(-) diff --git a/src/liballoc/collections/vec_deque.rs b/src/liballoc/collections/vec_deque.rs index 7307aad8a9ed6..de92c4997e371 100644 --- a/src/liballoc/collections/vec_deque.rs +++ b/src/liballoc/collections/vec_deque.rs @@ -2575,47 +2575,61 @@ unsafe impl Send for Drain<'_, T> {} #[stable(feature = "drain", since = "1.6.0")] impl Drop for Drain<'_, T> { fn drop(&mut self) { - self.for_each(drop); + struct DropGuard<'r, 'a, T>(&'r mut Drain<'a, T>); - let source_deque = unsafe { self.deque.as_mut() }; + impl<'r, 'a, T> Drop for DropGuard<'r, 'a, T> { + fn drop(&mut self) { + self.0.for_each(drop); - // T = source_deque_tail; H = source_deque_head; t = drain_tail; h = drain_head - // - // T t h H - // [. . . o o x x o o . . .] - // - let orig_tail = source_deque.tail; - let drain_tail = source_deque.head; - let drain_head = self.after_tail; - let orig_head = self.after_head; + let source_deque = unsafe { self.0.deque.as_mut() }; - let tail_len = count(orig_tail, drain_tail, source_deque.cap()); - let head_len = count(drain_head, orig_head, source_deque.cap()); + // T = source_deque_tail; H = source_deque_head; t = drain_tail; h = drain_head + // + // T t h H + // [. . . o o x x o o . . .] + // + let orig_tail = source_deque.tail; + let drain_tail = source_deque.head; + let drain_head = self.0.after_tail; + let orig_head = self.0.after_head; - // Restore the original head value - source_deque.head = orig_head; + let tail_len = count(orig_tail, drain_tail, source_deque.cap()); + let head_len = count(drain_head, orig_head, source_deque.cap()); - match (tail_len, head_len) { - (0, 0) => { - source_deque.head = 0; - source_deque.tail = 0; - } - (0, _) => { - source_deque.tail = drain_head; - } - (_, 0) => { - source_deque.head = drain_tail; - } - _ => unsafe { - if tail_len <= head_len { - source_deque.tail = source_deque.wrap_sub(drain_head, tail_len); - source_deque.wrap_copy(source_deque.tail, orig_tail, tail_len); - } else { - source_deque.head = source_deque.wrap_add(drain_tail, head_len); - source_deque.wrap_copy(drain_tail, drain_head, head_len); + // Restore the original head value + source_deque.head = orig_head; + + match (tail_len, head_len) { + (0, 0) => { + source_deque.head = 0; + source_deque.tail = 0; + } + (0, _) => { + source_deque.tail = drain_head; + } + (_, 0) => { + source_deque.head = drain_tail; + } + _ => unsafe { + if tail_len <= head_len { + source_deque.tail = source_deque.wrap_sub(drain_head, tail_len); + source_deque.wrap_copy(source_deque.tail, orig_tail, tail_len); + } else { + source_deque.head = source_deque.wrap_add(drain_tail, head_len); + source_deque.wrap_copy(drain_tail, drain_head, head_len); + } + }, } - }, + } } + + while let Some(item) = self.next() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } + + DropGuard(self); } } diff --git a/src/liballoc/tests/vec_deque.rs b/src/liballoc/tests/vec_deque.rs index 2dc50d0c70e23..07f1f098954a9 100644 --- a/src/liballoc/tests/vec_deque.rs +++ b/src/liballoc/tests/vec_deque.rs @@ -1606,3 +1606,41 @@ fn truncate_leak() { assert_eq!(unsafe { DROPS }, 7); } + +#[test] +fn test_drain_leak() { + static mut DROPS: i32 = 0; + + #[derive(Debug, PartialEq)] + struct D(u32, bool); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + + if self.1 { + panic!("panic in `drop`"); + } + } + } + + let mut v = VecDeque::new(); + v.push_back(D(4, false)); + v.push_back(D(5, false)); + v.push_back(D(6, false)); + v.push_front(D(3, false)); + v.push_front(D(2, true)); + v.push_front(D(1, false)); + v.push_front(D(0, false)); + + catch_unwind(AssertUnwindSafe(|| { + v.drain(1..=4); + })).ok(); + + assert_eq!(unsafe { DROPS }, 4); + assert_eq!(v.len(), 3); + drop(v); + assert_eq!(unsafe { DROPS }, 7); +} From 163ed23f0081d7283ccaef39141bc29879260663 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 13 Dec 2019 21:34:30 +0100 Subject: [PATCH 06/11] Fix leak in vec::IntoIter when a destructor panics --- src/liballoc/tests/vec.rs | 29 +++++++++++++++++++++++++++++ src/liballoc/vec.rs | 4 +++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 80acba0a3a162..2a027fd5d2b6e 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -765,6 +765,35 @@ fn test_into_iter_clone() { assert_eq!(it.next(), None); } +#[test] +fn test_into_iter_leak() { + static mut DROPS: i32 = 0; + + struct D(bool); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + + if self.0 { + panic!("panic in `drop`"); + } + } + } + + let v = vec![ + D(false), + D(true), + D(false), + ]; + + catch_unwind(move || drop(v.into_iter())).ok(); + + assert_eq!(unsafe { DROPS }, 3); +} + #[test] fn test_cow_from() { let borrowed: &[_] = &["borrowed", "(slice)"]; diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index ba71e42090cbb..6589cc5b1f675 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -2620,7 +2620,9 @@ impl Clone for IntoIter { unsafe impl<#[may_dangle] T> Drop for IntoIter { fn drop(&mut self) { // destroy the remaining elements - for _x in self.by_ref() {} + unsafe { + ptr::drop_in_place(self.as_mut_slice()); + } // RawVec handles deallocation let _ = unsafe { RawVec::from_raw_parts(self.buf.as_ptr(), self.cap) }; From 0ae16b47ffee866ee49f909ea213a28d8068cf56 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 14 Dec 2019 00:55:06 +0100 Subject: [PATCH 07/11] Avoid leak in DrainFilter when a drop panics --- src/liballoc/collections/linked_list.rs | 16 ++++++++++- src/liballoc/tests/linked_list.rs | 36 ++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/liballoc/collections/linked_list.rs b/src/liballoc/collections/linked_list.rs index b88ca8a0fb0d1..463ec67d66894 100644 --- a/src/liballoc/collections/linked_list.rs +++ b/src/liballoc/collections/linked_list.rs @@ -1565,7 +1565,21 @@ where F: FnMut(&mut T) -> bool, { fn drop(&mut self) { - self.for_each(drop); + struct DropGuard<'r, 'a, T, F>(&'r mut DrainFilter<'a, T, F>) + where F: FnMut(&mut T) -> bool; + + impl<'r, 'a, T, F> Drop for DropGuard<'r, 'a, T, F> + where F: FnMut(&mut T) -> bool { + fn drop(&mut self) { + self.0.for_each(drop); + } + } + + while let Some(item) = self.next() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } } } diff --git a/src/liballoc/tests/linked_list.rs b/src/liballoc/tests/linked_list.rs index b7736515b262a..aa3f582a165fb 100644 --- a/src/liballoc/tests/linked_list.rs +++ b/src/liballoc/tests/linked_list.rs @@ -1,5 +1,5 @@ use std::collections::LinkedList; -use std::panic::catch_unwind; +use std::panic::{catch_unwind, AssertUnwindSafe}; #[test] fn test_basic() { @@ -531,6 +531,40 @@ fn drain_filter_complex() { } } +#[test] +fn drain_filter_drop_panic_leak() { + static mut DROPS: i32 = 0; + + struct D(bool); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + + if self.0 { + panic!("panic in `drop`"); + } + } + } + + let mut q = LinkedList::new(); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_back(D(false)); + q.push_front(D(false)); + q.push_front(D(true)); + q.push_front(D(false)); + + catch_unwind(AssertUnwindSafe(|| drop(q.drain_filter(|_| true)))).ok(); + + assert_eq!(unsafe { DROPS }, 8); + assert!(q.is_empty()); +} + #[test] fn test_drop() { static mut DROPS: i32 = 0; From 1f373f4aeb279c6bf98976aee419db967c4f56f6 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 14 Dec 2019 01:01:28 +0100 Subject: [PATCH 08/11] Add test for panic in LL DrainFilter predicate --- src/liballoc/tests/linked_list.rs | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/liballoc/tests/linked_list.rs b/src/liballoc/tests/linked_list.rs index aa3f582a165fb..c40f99ee9069d 100644 --- a/src/liballoc/tests/linked_list.rs +++ b/src/liballoc/tests/linked_list.rs @@ -565,6 +565,41 @@ fn drain_filter_drop_panic_leak() { assert!(q.is_empty()); } +#[test] +fn drain_filter_pred_panic_leak() { + static mut DROPS: i32 = 0; + + #[derive(Debug)] + struct D(u32); + + impl Drop for D { + fn drop(&mut self) { + unsafe { + DROPS += 1; + } + } + } + + let mut q = LinkedList::new(); + q.push_back(D(3)); + q.push_back(D(4)); + q.push_back(D(5)); + q.push_back(D(6)); + q.push_back(D(7)); + q.push_front(D(2)); + q.push_front(D(1)); + q.push_front(D(0)); + + catch_unwind(AssertUnwindSafe(|| drop(q.drain_filter(|item| if item.0 >= 2 { + panic!() + } else { + true + })))).ok(); + + assert_eq!(unsafe { DROPS }, 2); // 0 and 1 + assert_eq!(q.len(), 6); +} + #[test] fn test_drop() { static mut DROPS: i32 = 0; From 75f721df97fd7895b31a1d8c9ed05a368fc95d6d Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 14 Dec 2019 17:06:22 +0100 Subject: [PATCH 09/11] Move VecDeque Drain iterator to new file --- src/liballoc/collections/vec_deque.rs | 126 +------------------- src/liballoc/collections/vec_deque/drain.rs | 126 ++++++++++++++++++++ 2 files changed, 131 insertions(+), 121 deletions(-) create mode 100644 src/liballoc/collections/vec_deque/drain.rs diff --git a/src/liballoc/collections/vec_deque.rs b/src/liballoc/collections/vec_deque.rs index de92c4997e371..85d1d98b8a9c2 100644 --- a/src/liballoc/collections/vec_deque.rs +++ b/src/liballoc/collections/vec_deque.rs @@ -22,6 +22,11 @@ use crate::collections::TryReserveError; use crate::raw_vec::RawVec; use crate::vec::Vec; +#[stable(feature = "drain", since = "1.6.0")] +pub use self::drain::Drain; + +mod drain; + #[cfg(test)] mod tests; @@ -2541,127 +2546,6 @@ impl ExactSizeIterator for IntoIter { #[stable(feature = "fused", since = "1.26.0")] impl FusedIterator for IntoIter {} -/// A draining iterator over the elements of a `VecDeque`. -/// -/// This `struct` is created by the [`drain`] method on [`VecDeque`]. See its -/// documentation for more. -/// -/// [`drain`]: struct.VecDeque.html#method.drain -/// [`VecDeque`]: struct.VecDeque.html -#[stable(feature = "drain", since = "1.6.0")] -pub struct Drain<'a, T: 'a> { - after_tail: usize, - after_head: usize, - iter: Iter<'a, T>, - deque: NonNull>, -} - -#[stable(feature = "collection_debug", since = "1.17.0")] -impl fmt::Debug for Drain<'_, T> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_tuple("Drain") - .field(&self.after_tail) - .field(&self.after_head) - .field(&self.iter) - .finish() - } -} - -#[stable(feature = "drain", since = "1.6.0")] -unsafe impl Sync for Drain<'_, T> {} -#[stable(feature = "drain", since = "1.6.0")] -unsafe impl Send for Drain<'_, T> {} - -#[stable(feature = "drain", since = "1.6.0")] -impl Drop for Drain<'_, T> { - fn drop(&mut self) { - struct DropGuard<'r, 'a, T>(&'r mut Drain<'a, T>); - - impl<'r, 'a, T> Drop for DropGuard<'r, 'a, T> { - fn drop(&mut self) { - self.0.for_each(drop); - - let source_deque = unsafe { self.0.deque.as_mut() }; - - // T = source_deque_tail; H = source_deque_head; t = drain_tail; h = drain_head - // - // T t h H - // [. . . o o x x o o . . .] - // - let orig_tail = source_deque.tail; - let drain_tail = source_deque.head; - let drain_head = self.0.after_tail; - let orig_head = self.0.after_head; - - let tail_len = count(orig_tail, drain_tail, source_deque.cap()); - let head_len = count(drain_head, orig_head, source_deque.cap()); - - // Restore the original head value - source_deque.head = orig_head; - - match (tail_len, head_len) { - (0, 0) => { - source_deque.head = 0; - source_deque.tail = 0; - } - (0, _) => { - source_deque.tail = drain_head; - } - (_, 0) => { - source_deque.head = drain_tail; - } - _ => unsafe { - if tail_len <= head_len { - source_deque.tail = source_deque.wrap_sub(drain_head, tail_len); - source_deque.wrap_copy(source_deque.tail, orig_tail, tail_len); - } else { - source_deque.head = source_deque.wrap_add(drain_tail, head_len); - source_deque.wrap_copy(drain_tail, drain_head, head_len); - } - }, - } - } - } - - while let Some(item) = self.next() { - let guard = DropGuard(self); - drop(item); - mem::forget(guard); - } - - DropGuard(self); - } -} - -#[stable(feature = "drain", since = "1.6.0")] -impl Iterator for Drain<'_, T> { - type Item = T; - - #[inline] - fn next(&mut self) -> Option { - self.iter.next().map(|elt| unsafe { ptr::read(elt) }) - } - - #[inline] - fn size_hint(&self) -> (usize, Option) { - self.iter.size_hint() - } -} - -#[stable(feature = "drain", since = "1.6.0")] -impl DoubleEndedIterator for Drain<'_, T> { - #[inline] - fn next_back(&mut self) -> Option { - self.iter.next_back().map(|elt| unsafe { ptr::read(elt) }) - } -} - -#[stable(feature = "drain", since = "1.6.0")] -impl ExactSizeIterator for Drain<'_, T> {} - -#[stable(feature = "fused", since = "1.26.0")] -impl FusedIterator for Drain<'_, T> {} - #[stable(feature = "rust1", since = "1.0.0")] impl PartialEq for VecDeque { fn eq(&self, other: &VecDeque) -> bool { diff --git a/src/liballoc/collections/vec_deque/drain.rs b/src/liballoc/collections/vec_deque/drain.rs new file mode 100644 index 0000000000000..1ae94de75adb7 --- /dev/null +++ b/src/liballoc/collections/vec_deque/drain.rs @@ -0,0 +1,126 @@ +use core::iter::FusedIterator; +use core::ptr::{self, NonNull}; +use core::{fmt, mem}; + +use super::{count, Iter, VecDeque}; + +/// A draining iterator over the elements of a `VecDeque`. +/// +/// This `struct` is created by the [`drain`] method on [`VecDeque`]. See its +/// documentation for more. +/// +/// [`drain`]: struct.VecDeque.html#method.drain +/// [`VecDeque`]: struct.VecDeque.html +#[stable(feature = "drain", since = "1.6.0")] +pub struct Drain<'a, T: 'a> { + pub(crate) after_tail: usize, + pub(crate) after_head: usize, + pub(crate) iter: Iter<'a, T>, + pub(crate) deque: NonNull>, +} + +#[stable(feature = "collection_debug", since = "1.17.0")] +impl fmt::Debug for Drain<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("Drain") + .field(&self.after_tail) + .field(&self.after_head) + .field(&self.iter) + .finish() + } +} + +#[stable(feature = "drain", since = "1.6.0")] +unsafe impl Sync for Drain<'_, T> {} +#[stable(feature = "drain", since = "1.6.0")] +unsafe impl Send for Drain<'_, T> {} + +#[stable(feature = "drain", since = "1.6.0")] +impl Drop for Drain<'_, T> { + fn drop(&mut self) { + struct DropGuard<'r, 'a, T>(&'r mut Drain<'a, T>); + + impl<'r, 'a, T> Drop for DropGuard<'r, 'a, T> { + fn drop(&mut self) { + self.0.for_each(drop); + + let source_deque = unsafe { self.0.deque.as_mut() }; + + // T = source_deque_tail; H = source_deque_head; t = drain_tail; h = drain_head + // + // T t h H + // [. . . o o x x o o . . .] + // + let orig_tail = source_deque.tail; + let drain_tail = source_deque.head; + let drain_head = self.0.after_tail; + let orig_head = self.0.after_head; + + let tail_len = count(orig_tail, drain_tail, source_deque.cap()); + let head_len = count(drain_head, orig_head, source_deque.cap()); + + // Restore the original head value + source_deque.head = orig_head; + + match (tail_len, head_len) { + (0, 0) => { + source_deque.head = 0; + source_deque.tail = 0; + } + (0, _) => { + source_deque.tail = drain_head; + } + (_, 0) => { + source_deque.head = drain_tail; + } + _ => unsafe { + if tail_len <= head_len { + source_deque.tail = source_deque.wrap_sub(drain_head, tail_len); + source_deque.wrap_copy(source_deque.tail, orig_tail, tail_len); + } else { + source_deque.head = source_deque.wrap_add(drain_tail, head_len); + source_deque.wrap_copy(drain_tail, drain_head, head_len); + } + }, + } + } + } + + while let Some(item) = self.next() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } + + DropGuard(self); + } +} + +#[stable(feature = "drain", since = "1.6.0")] +impl Iterator for Drain<'_, T> { + type Item = T; + + #[inline] + fn next(&mut self) -> Option { + self.iter.next().map(|elt| unsafe { ptr::read(elt) }) + } + + #[inline] + fn size_hint(&self) -> (usize, Option) { + self.iter.size_hint() + } +} + +#[stable(feature = "drain", since = "1.6.0")] +impl DoubleEndedIterator for Drain<'_, T> { + #[inline] + fn next_back(&mut self) -> Option { + self.iter.next_back().map(|elt| unsafe { ptr::read(elt) }) + } +} + +#[stable(feature = "drain", since = "1.6.0")] +impl ExactSizeIterator for Drain<'_, T> {} + +#[stable(feature = "fused", since = "1.26.0")] +impl FusedIterator for Drain<'_, T> {} From 52d6c90488abdd12a24c66f5e3490ae3136bb295 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 16 Dec 2019 21:42:46 +0100 Subject: [PATCH 10/11] Update comments in `Drain`s `Drop` impl --- src/liballoc/vec.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 6589cc5b1f675..4fa60846e22a6 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -2702,13 +2702,14 @@ impl DoubleEndedIterator for Drain<'_, T> { #[stable(feature = "drain", since = "1.6.0")] impl Drop for Drain<'_, T> { fn drop(&mut self) { - /// Continues dropping the remaining elements when a destructor unwinds. + /// Continues dropping the remaining elements in the `Drain`, then moves back the + /// un-`Drain`ed elements to restore the original `Vec`. struct DropGuard<'r, 'a, T>(&'r mut Drain<'a, T>); impl<'r, 'a, T> Drop for DropGuard<'r, 'a, T> { fn drop(&mut self) { - // Continue the same loop we do below. This only runs when a destructor has - // panicked. If another one panics this will abort. + // Continue the same loop we have below. If the loop already finished, this does + // nothing. self.0.for_each(drop); if self.0.tail_len > 0 { @@ -2735,6 +2736,7 @@ impl Drop for Drain<'_, T> { mem::forget(guard); } + // Drop a `DropGuard` to move back the non-drained tail of `self`. DropGuard(self); } } From e5987a062f487321bdfcbbdac4b0b30548258631 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 19 Jan 2020 20:50:00 +0100 Subject: [PATCH 11/11] Format --- src/liballoc/collections/linked_list.rs | 7 +++++-- src/liballoc/tests/linked_list.rs | 11 +++++------ src/liballoc/tests/vec.rs | 6 +----- src/liballoc/tests/vec_deque.rs | 5 +++-- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/liballoc/collections/linked_list.rs b/src/liballoc/collections/linked_list.rs index 463ec67d66894..399dbe5e4bf2f 100644 --- a/src/liballoc/collections/linked_list.rs +++ b/src/liballoc/collections/linked_list.rs @@ -1566,10 +1566,13 @@ where { fn drop(&mut self) { struct DropGuard<'r, 'a, T, F>(&'r mut DrainFilter<'a, T, F>) - where F: FnMut(&mut T) -> bool; + where + F: FnMut(&mut T) -> bool; impl<'r, 'a, T, F> Drop for DropGuard<'r, 'a, T, F> - where F: FnMut(&mut T) -> bool { + where + F: FnMut(&mut T) -> bool, + { fn drop(&mut self) { self.0.for_each(drop); } diff --git a/src/liballoc/tests/linked_list.rs b/src/liballoc/tests/linked_list.rs index c40f99ee9069d..afcb9e03fd097 100644 --- a/src/liballoc/tests/linked_list.rs +++ b/src/liballoc/tests/linked_list.rs @@ -590,13 +590,12 @@ fn drain_filter_pred_panic_leak() { q.push_front(D(1)); q.push_front(D(0)); - catch_unwind(AssertUnwindSafe(|| drop(q.drain_filter(|item| if item.0 >= 2 { - panic!() - } else { - true - })))).ok(); + catch_unwind(AssertUnwindSafe(|| { + drop(q.drain_filter(|item| if item.0 >= 2 { panic!() } else { true })) + })) + .ok(); - assert_eq!(unsafe { DROPS }, 2); // 0 and 1 + assert_eq!(unsafe { DROPS }, 2); // 0 and 1 assert_eq!(q.len(), 6); } diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 2a027fd5d2b6e..9c4ac52acac2a 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -783,11 +783,7 @@ fn test_into_iter_leak() { } } - let v = vec![ - D(false), - D(true), - D(false), - ]; + let v = vec![D(false), D(true), D(false)]; catch_unwind(move || drop(v.into_iter())).ok(); diff --git a/src/liballoc/tests/vec_deque.rs b/src/liballoc/tests/vec_deque.rs index 07f1f098954a9..101dd67d97a9a 100644 --- a/src/liballoc/tests/vec_deque.rs +++ b/src/liballoc/tests/vec_deque.rs @@ -2,7 +2,7 @@ use std::collections::TryReserveError::*; use std::collections::{vec_deque::Drain, VecDeque}; use std::fmt::Debug; use std::mem::size_of; -use std::panic::{AssertUnwindSafe, catch_unwind}; +use std::panic::{catch_unwind, AssertUnwindSafe}; use std::{isize, usize}; use crate::hash; @@ -1637,7 +1637,8 @@ fn test_drain_leak() { catch_unwind(AssertUnwindSafe(|| { v.drain(1..=4); - })).ok(); + })) + .ok(); assert_eq!(unsafe { DROPS }, 4); assert_eq!(v.len(), 3);