diff --git a/src/linked_hash_map.rs b/src/linked_hash_map.rs index d2f021d..2a442d6 100644 --- a/src/linked_hash_map.rs +++ b/src/linked_hash_map.rs @@ -106,13 +106,9 @@ impl LinkedHashMap { #[inline] pub fn clear(&mut self) { self.table.clear(); - if let Some(mut values) = self.values { + if let Some(values) = self.values { unsafe { drop_value_nodes(values); - values.as_mut().links.value = ValueLinks { - prev: values, - next: values, - }; } } } @@ -2228,13 +2224,48 @@ unsafe fn allocate_node(free_list: &mut Option>>) -> No // Given node is assumed to be the guard node and is *not* dropped. #[inline] -unsafe fn drop_value_nodes(guard: NonNull>) { - let mut cur = guard.as_ref().links.value.prev; - while cur != guard { - let prev = cur.as_ref().links.value.prev; - cur.as_mut().take_entry(); - let _ = Box::from_raw(cur.as_ptr()); - cur = prev; +unsafe fn drop_value_nodes(mut guard: NonNull>) { + // Detach the value list from the guard before dropping any nodes, so the + // guard is always left as an empty, consistent list. This matters when an + // entry's `Drop` panics: without it, a caught panic (e.g. via `clear`) + // could observe a node whose entry was already moved out and drop it again. + let cur = guard.as_ref().links.value.prev; + guard.as_mut().links.value = ValueLinks { + prev: guard, + next: guard, + }; + + // `Remainder` owns the not-yet-freed tail of the detached chain. If + // dropping an entry panics, its `Drop` frees the remaining nodes during + // unwinding (rather than leaking them), matching how the standard library + // drops the rest of a collection when one element's destructor panics. + struct Remainder { + cur: NonNull>, + guard: NonNull>, + } + + impl Drop for Remainder { + fn drop(&mut self) { + while self.cur != self.guard { + unsafe { + let prev = self.cur.as_ref().links.value.prev; + let _ = self.cur.as_mut().take_entry(); + let _ = Box::from_raw(self.cur.as_ptr()); + self.cur = prev; + } + } + } + } + + let mut rem = Remainder { cur, guard }; + while rem.cur != guard { + let prev = rem.cur.as_ref().links.value.prev; + let entry = rem.cur.as_mut().take_entry(); + // Free the node and advance past it before dropping the entry, so that + // if the entry's `Drop` panics, `Remainder` resumes from the next node. + let _ = Box::from_raw(rem.cur.as_ptr()); + rem.cur = prev; + drop(entry); } } diff --git a/tests/linked_hash_map.rs b/tests/linked_hash_map.rs index 5317595..911b5b6 100644 --- a/tests/linked_hash_map.rs +++ b/tests/linked_hash_map.rs @@ -1,3 +1,9 @@ +use std::{ + cell::Cell, + panic::{catch_unwind, AssertUnwindSafe}, + rc::Rc, +}; + use hashlink::{linked_hash_map, LinkedHashMap}; #[allow(dead_code)] @@ -820,3 +826,45 @@ fn test_cursor_back_mut() { assert!(cursor.current().is_some()); assert_eq!(cursor.current().unwrap().1, &mut 3); } + +// Regression test for https://github.com/djc/hashlink/issues/43 +// +// A panic while dropping an entry during `clear` must not leave a moved-out node reachable +// from the value list, which would cause its entry to be dropped again. +#[test] +fn test_clear_panic_safe() { + struct PanicOnDrop { + should_panic: Rc>, + } + + impl Drop for PanicOnDrop { + fn drop(&mut self) { + if self.should_panic.replace(false) { + panic!("intentional panic while clearing LinkedHashMap"); + } + } + } + + let should_panic = Rc::new(Cell::new(true)); + let mut map = LinkedHashMap::new(); + map.insert( + 1, + PanicOnDrop { + should_panic: Rc::clone(&should_panic), + }, + ); + map.insert( + 2, + PanicOnDrop { + should_panic: Rc::clone(&should_panic), + }, + ); + + let result = catch_unwind(AssertUnwindSafe(|| map.clear())); + assert!(result.is_err()); + should_panic.set(false); + + // Dropping (or reusing) the map after the caught panic must not traverse a + // stale, moved-out node. + drop(map); +}