Fix panic-safety unsoundness in LinkedHashMap::into_iter and clear — #116

src/lib.rs

@@ -622,13 +622,26 @@ impl<K: Hash + Eq, V, S: BuildHasher> LinkedHashMap<K, V, S> {
         // update the guard node if present
         if !self.head.is_null() {
             unsafe {
-                self.drop_entries();
+                // Detach the guard links *before* freeing any node. Freeing the
+                // value nodes runs each `K`/`V`'s user-controlled `Drop`, which
+                // may panic. If that happens after some nodes are freed but
+                // before the guard is reset, `self.head` would still point at
+                // freed memory; the map's own `Drop` then re-walks the chain and
+                // frees it again -- a double-free reachable from safe Rust. By
+                // resetting the guard first, `self.head` describes an empty list
+                // as soon as unwinding can begin, so no node is freed twice
+                // (any unreached nodes simply leak, which is safe).
+                let mut cur = (*self.head).next;
                 (*self.head).prev = self.head;
                 (*self.head).next = self.head;
+                while cur != self.head {
+                    let next = (*cur).next;
+                    Box::from_raw(cur);
+                    cur = next;
+                }
             }
         }
     }
-
     /// Returns a double-ended iterator visiting all key-value pairs in order of insertion.
     /// Iterator element type is `(&'a K, &'a V)`
     ///
@@ -1407,23 +1420,32 @@ impl<'a, K: Hash + Eq, V, S: BuildHasher> IntoIterator for &'a mut LinkedHashMap
 impl<K: Hash + Eq, V, S: BuildHasher> IntoIterator for LinkedHashMap<K, V, S> {
     type Item = (K, V);
     type IntoIter = IntoIter<K, V>;
-    fn into_iter(mut self) -> IntoIter<K, V> {
-        let (head, tail) = if !self.head.is_null() {
-            unsafe { ((*self.head).prev, (*self.head).next) }
+    fn into_iter(self) -> IntoIter<K, V> {
+        // Wrap `self` in `ManuallyDrop` up front. The teardown below can unwind
+        // (the internal HashMap owns the hasher `S`, whose `Drop` is
+        // user-controlled and may panic). If that happens after the sentinel
+        // node has already been freed, the original `LinkedHashMap` must NOT be
+        // dropped automatically -- its destructor would follow the now-dangling
+        // `head` pointer, a use-after-free. `ManuallyDrop` suppresses that drop
+        // unconditionally, instead of relying on a `mem::forget(self)` that is
+        // skipped when the teardown unwinds.
+        let mut this = mem::ManuallyDrop::new(self);
+
+        let (head, tail) = if !this.head.is_null() {
+            unsafe { ((*this.head).prev, (*this.head).next) }
         } else {
             (ptr::null_mut(), ptr::null_mut())
         };
-        let len = self.len();
+        let len = this.len();
 
-        if !self.head.is_null() {
-            unsafe { drop_empty_node(self.head) }
+        if !this.head.is_null() {
+            unsafe { drop_empty_node(this.head) }
         }
-        self.clear_free_list();
+        this.clear_free_list();
         // drop the HashMap but not the LinkedHashMap
         unsafe {
-            ptr::drop_in_place(&mut self.map);
+            ptr::drop_in_place(&mut this.map);
         }
-        mem::forget(self);
 
         IntoIter {
             head,

src/tests/mod.rs

@@ -878,3 +878,109 @@ mod bench {
         })
     }
 }
+
+// Regression test for panic-safety in `into_iter`. The consuming iterator
+// tears the map down manually (freeing the sentinel node, then dropping the
+// internal HashMap) before committing the ownership transfer. The HashMap owns
+// the hasher `S`, whose `Drop` is user-controlled and may panic. If it panics
+// after the sentinel has been freed, the original map must not be dropped
+// automatically -- doing so would follow the now-dangling head pointer, a
+// use-after-free. This test drives exactly that path under `catch_unwind`; with
+// the fix it unwinds cleanly (no double-free / UAF). Any leak of the remaining
+// nodes is acceptable: leaking on panic is safe, unlike memory corruption.
+#[test]
+fn into_iter_panicking_hasher_is_sound() {
+    use std::collections::hash_map::DefaultHasher;
+    use std::hash::BuildHasher;
+    use std::panic::{catch_unwind, AssertUnwindSafe};
+    use std::sync::atomic::{AtomicBool, Ordering};
+
+    // A BuildHasher whose Drop panics exactly once.
+    struct PanicHasher {
+        armed: AtomicBool,
+    }
+
+    impl BuildHasher for PanicHasher {
+        type Hasher = DefaultHasher;
+        fn build_hasher(&self) -> DefaultHasher {
+            DefaultHasher::new()
+        }
+    }
+
+    impl Drop for PanicHasher {
+        fn drop(&mut self) {
+            // Panic only once; a second panic during unwinding would abort.
+            if self.armed.swap(false, Ordering::SeqCst) {
+                panic!("hasher drop panic");
+            }
+        }
+    }
+
+    let hasher = PanicHasher {
+        armed: AtomicBool::new(true),
+    };
+    let mut map = LinkedHashMap::with_hasher(hasher);
+    // Heap-owning entries so a re-drop would be a real use-after-free.
+    map.insert("key-0".to_string(), "value-0".to_string());
+    map.insert("key-1".to_string(), "value-1".to_string());
+
+    // Before the fix, the hasher's panic while `into_iter` drops the internal
+    // map left the original LinkedHashMap to be dropped during unwinding, whose
+    // destructor followed the freed sentinel -> heap-use-after-free (ASAN). The
+    // fix wraps `self` in ManuallyDrop, so the panic unwinds without that
+    // second destruction. Reaching this assertion without aborting is the point.
+    let result = catch_unwind(AssertUnwindSafe(|| {
+        let _ = map.into_iter();
+    }));
+
+    assert!(result.is_err(), "expected the hasher Drop to unwind");
+}
+
+// Regression test for panic-safety in `clear`. `clear` frees the value nodes
+// (running each `K`/`V`'s user-controlled `Drop`) and only afterwards resets
+// the sentinel's guard links. If a value's `Drop` panics partway through, the
+// reset is skipped and `self.head` is left pointing at freed memory; the map's
+// own destructor then re-walks the chain during unwinding and frees it again
+// -- a double-free (CWE-415) reachable from safe Rust. This test drives that
+// path under `catch_unwind`; with the fix `clear` detaches the guard links
+// before freeing, so unwinding leaves an empty, self-consistent list (leaking
+// the unreached nodes, which is safe).
+#[test]
+fn clear_panicking_value_drop_is_sound() {
+    use std::panic::{catch_unwind, AssertUnwindSafe};
+    use std::sync::atomic::{AtomicBool, Ordering};
+
+    struct PanicOnDrop {
+        armed: AtomicBool,
+        _heap: String,
+    }
+
+    impl Drop for PanicOnDrop {
+        fn drop(&mut self) {
+            // Panic only once; a second panic during unwinding would abort.
+            if self.armed.swap(false, Ordering::SeqCst) {
+                panic!("value drop panic");
+            }
+        }
+    }
+
+    let mut map: LinkedHashMap<String, PanicOnDrop> = LinkedHashMap::new();
+    // Heap-owning entries so a re-drop would be a real use-after-free.
+    for i in 0..4 {
+        map.insert(
+            format!("key-{i}"),
+            PanicOnDrop {
+                armed: AtomicBool::new(i == 3),
+                _heap: "x".repeat(64),
+            },
+        );
+    }
+
+    let result = catch_unwind(AssertUnwindSafe(|| map.clear()));
+    assert!(result.is_err(), "expected the value Drop to unwind");
+
+    // Before the fix, the panic left `self.head` dangling; dropping the map
+    // here re-freed the chain -> double-free. With the fix the map is already
+    // an empty, self-consistent list, so this is sound.
+    drop(map);
+}