Skip to content

Commit

Permalink
Auto merge of #67290 - jonas-schievink:leak-audit, r=KodrAus
Browse files Browse the repository at this point in the history
Audit liballoc for leaks in `Drop` impls when user destructor panics

Inspired by #67243 and #67235, this audits and hopefully fixes the remaining `Drop` impls in liballoc for resource leaks in the presence of panics in destructors called by the affected `Drop` impl.

This does not touch `Hash{Map,Set}` since they live in hashbrown. They have similar issues though.

r? @KodrAus
  • Loading branch information
bors committed Feb 26, 2020
2 parents 3a0d106 + e5987a0 commit 892cb14
Show file tree
Hide file tree
Showing 11 changed files with 497 additions and 128 deletions.
16 changes: 14 additions & 2 deletions src/liballoc/collections/binary_heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
}
}

Expand Down
17 changes: 16 additions & 1 deletion src/liballoc/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1470,7 +1470,22 @@ impl<K, V> IntoIterator for BTreeMap<K, V> {
#[stable(feature = "btree_drop", since = "1.7.0")]
impl<K, V> Drop for IntoIter<K, V> {
fn drop(&mut self) {
self.for_each(drop);
struct DropGuard<'a, K, V>(&'a mut IntoIter<K, V>);

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() {
Expand Down
19 changes: 18 additions & 1 deletion src/liballoc/collections/linked_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1611,7 +1611,24 @@ 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);
}
}
}

Expand Down
129 changes: 21 additions & 108 deletions src/liballoc/collections/vec_deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -866,6 +871,18 @@ impl<T> VecDeque<T> {
/// ```
#[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
Expand All @@ -888,8 +905,11 @@ impl<T> VecDeque<T> {
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);
}
}
}
Expand Down Expand Up @@ -2526,113 +2546,6 @@ impl<T> ExactSizeIterator for IntoIter<T> {
#[stable(feature = "fused", since = "1.26.0")]
impl<T> FusedIterator for IntoIter<T> {}

/// 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<VecDeque<T>>,
}

#[stable(feature = "collection_debug", since = "1.17.0")]
impl<T: fmt::Debug> 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<T: Sync> Sync for Drain<'_, T> {}
#[stable(feature = "drain", since = "1.6.0")]
unsafe impl<T: Send> Send for Drain<'_, T> {}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> Drop for Drain<'_, T> {
fn drop(&mut self) {
self.for_each(drop);

let source_deque = unsafe { self.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.after_tail;
let orig_head = self.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);
}
},
}
}
}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> Iterator for Drain<'_, T> {
type Item = T;

#[inline]
fn next(&mut self) -> Option<T> {
self.iter.next().map(|elt| unsafe { ptr::read(elt) })
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
self.iter.size_hint()
}
}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> DoubleEndedIterator for Drain<'_, T> {
#[inline]
fn next_back(&mut self) -> Option<T> {
self.iter.next_back().map(|elt| unsafe { ptr::read(elt) })
}
}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> ExactSizeIterator for Drain<'_, T> {}

#[stable(feature = "fused", since = "1.26.0")]
impl<T> FusedIterator for Drain<'_, T> {}

#[stable(feature = "rust1", since = "1.0.0")]
impl<A: PartialEq> PartialEq for VecDeque<A> {
fn eq(&self, other: &VecDeque<A>) -> bool {
Expand Down
126 changes: 126 additions & 0 deletions src/liballoc/collections/vec_deque/drain.rs
Original file line number Diff line number Diff line change
@@ -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<VecDeque<T>>,
}

#[stable(feature = "collection_debug", since = "1.17.0")]
impl<T: fmt::Debug> 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<T: Sync> Sync for Drain<'_, T> {}
#[stable(feature = "drain", since = "1.6.0")]
unsafe impl<T: Send> Send for Drain<'_, T> {}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> 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<T> Iterator for Drain<'_, T> {
type Item = T;

#[inline]
fn next(&mut self) -> Option<T> {
self.iter.next().map(|elt| unsafe { ptr::read(elt) })
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
self.iter.size_hint()
}
}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> DoubleEndedIterator for Drain<'_, T> {
#[inline]
fn next_back(&mut self) -> Option<T> {
self.iter.next_back().map(|elt| unsafe { ptr::read(elt) })
}
}

#[stable(feature = "drain", since = "1.6.0")]
impl<T> ExactSizeIterator for Drain<'_, T> {}

#[stable(feature = "fused", since = "1.26.0")]
impl<T> FusedIterator for Drain<'_, T> {}
Loading

0 comments on commit 892cb14

Please sign in to comment.