diff --git a/src/internal/linked_list.rs b/src/internal/linked_list.rs index cd12e41..b3473ac 100644 --- a/src/internal/linked_list.rs +++ b/src/internal/linked_list.rs @@ -3,7 +3,6 @@ use std::alloc::dealloc; use std::alloc::handle_alloc_error; use std::alloc::Layout; use std::marker::PhantomData; -use std::mem; use std::ptr::NonNull; use std::ptr::{self}; @@ -212,20 +211,23 @@ impl LinkedList { fn drop_nodes(&mut self) { let s = self.sentinel.as_ptr(); while self.len != 0 { - unsafe { + let element = unsafe { // SAFETY: `self.len != 0`, so `(*s).next` points to a real node. let node = (*s).next; let second = (*node).next; (*second).prev = s; (*s).next = second; - let node_ptr = node as *mut Node; // SAFETY: layout-compatible (see `Node` docs); `element` still init. - ptr::drop_in_place(&mut (*node_ptr).element); - // SAFETY: element dropped above; node already unlinked. + let element = ptr::read(&(*(node as *mut Node)).element); + // SAFETY: element moved out above; node already unlinked. self.recycle_node(node); - } + element + }; self.len -= 1; + // Dropped last: if this panics, the list is already consistent and + // the drop guard can carry on. + drop(element); } } @@ -439,17 +441,22 @@ impl LinkedList { } } -// Guard ensures that if dropping an element panics, we still free -// the free-list allocations instead of leaking them (the sentinel -// is freed unconditionally below regardless of panics). +// Frees the rest of the list when it goes out of scope, which also covers an +// element destructor panicking in `LinkedList::drop`: the remaining elements, +// the free list and the sentinel are freed either way. struct DropGuard<'a, T>(&'a mut LinkedList); impl<'a, T> Drop for DropGuard<'a, T> { fn drop(&mut self) { self.0.drop_nodes(); - // SAFETY: `self` is being torn down during unwind; no other - // references to the free list exist, satisfying `drop_freelist`'s contract. - unsafe { self.0.drop_freelist() }; + unsafe { + // SAFETY: the list is being destroyed; no other references to the + // free list exist, satisfying `drop_freelist`'s contract. + self.0.drop_freelist(); + // SAFETY: `sentinel` was allocated in `new` with exactly this + // layout and is never dereferenced after this point. + dealloc(self.0.sentinel.as_ptr() as *mut u8, Layout::new::()); + } } } @@ -457,16 +464,7 @@ impl Drop for LinkedList { fn drop(&mut self) { let guard = DropGuard(self); guard.0.drop_nodes(); - // SAFETY: `drop_nodes` completed without panicking; the free list is - // only touched here and then never again (guard is forgotten next). - unsafe { guard.0.drop_freelist() }; - mem::forget(guard); - // SAFETY: `self.sentinel` was allocated in `new` with exactly this - // layout (`Layout::new::()`), and after this point `self` is - // being destroyed, so the pointer is never dereferenced again. - unsafe { - dealloc(self.sentinel.as_ptr() as *mut u8, Layout::new::()); - } + // `guard` drops here and frees the free list and the sentinel. } } @@ -659,3 +657,36 @@ unsafe impl Send for RawIter {} unsafe impl Sync for RawIter {} unsafe impl Send for Cursor {} unsafe impl Sync for Cursor {} + +#[cfg(test)] +mod tests { + use super::LinkedList; + + #[test] + fn a_panicking_element_drop_still_frees_every_node() { + struct Bomb { + panics: bool, + _label: String, + } + impl Drop for Bomb { + fn drop(&mut self) { + if self.panics { + panic!("boom"); + } + } + } + + let mut list = LinkedList::new(); + for i in 0..6 { + list.push_back(Bomb { + panics: i == 2, + _label: i.to_string(), + }); + } + list.pop_back(); // leave a node on the free list too + let dropped = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || drop(list))); + assert!(dropped.is_err()); + // the other elements, the free list and the sentinel are freed; under + // Miri this also checks that nothing leaks + } +}