Fix UB in LinkedHashMap::clear()

src/linked_hash_map.rs

@@ -106,13 +106,9 @@ impl<K, V, S> LinkedHashMap<K, V, S> {
     #[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<K, V>(free_list: &mut Option<NonNull<Node<K, V>>>) -> No
 
 // Given node is assumed to be the guard node and is *not* dropped.
 #[inline]
-unsafe fn drop_value_nodes<K, V>(guard: NonNull<Node<K, V>>) {
-    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<K, V>(mut guard: NonNull<Node<K, V>>) {
+    // 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<K, V> {
+        cur: NonNull<Node<K, V>>,
+        guard: NonNull<Node<K, V>>,
+    }
+
+    impl<K, V> Drop for Remainder<K, V> {
+        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);
     }
 }
 

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<Cell<bool>>,
+    }
+
+    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);
+}