Fix panic-safety of eleven methods, and Deque's ring buffer indexing

CHANGELOG.md

@@ -10,6 +10,15 @@
 ## Bugfixes
 
 - Relax unnecessarily strict trait bounds on `{AllocVec, AllocDeque, AllocHeap}::{with_capacity, clone}`.
+- Fix `Deque::{truncate, retain}` addressing the ring buffer without accounting
+  for the front offset, which corrupted deques that had ever been pushed to at
+  the front.
+- Make `Vec::{truncate, clear}`, `Deque::{truncate, clear, retain}`,
+  `ListMap::{clear, remove}`, `PackedPool::clear`, `UnitCache::clear`,
+  `LruCache2::get_or_insert_with`, `OptionGroup::clear` and
+  `DirectPool::try_insert_with_handle`
+  panic-safe; a user-supplied destructor, predicate or closure that unwinds no
+  longer leaves the collection claiming values it does not own.
 
 # 0.3.0 (2022-03-04)
 ## Breaking Changes
@@ -68,4 +77,4 @@
 
 # 0.1.0 (2020-12-03)
 
-Initial Release
+Initial Release

src/collections/cache.rs

@@ -159,12 +159,16 @@ impl<K: Eq, V> CacheLine<K, V> for UnitCache<K, V> {
             return;
         }
 
+        // Clear the flag before destroying anything: a destructor may unwind,
+        // and `Drop for UnitCache` would then destroy both halves of the entry
+        // a second time.
+        self.occupied = false;
+
         unsafe {
-            self.key.as_mut_ptr().drop_in_place();
-            self.value.as_mut_ptr().drop_in_place();
+            let key = self.key.as_ptr().read();
+            let value = self.value.as_ptr().read();
+            drop((key, value));
         }
-
-        self.occupied = false;
     }
 }
 
@@ -335,14 +339,20 @@ impl<K: Eq, V> CacheLine<K, V> for LruCache2<K, V> {
             self.mark_used(lru);
 
             unsafe {
-                self.keys[lru].as_mut_ptr().drop_in_place();
-                self.values[lru].as_mut_ptr().drop_in_place();
-            }
+                // Move the evicted entry out and install the replacement before
+                // running any user code: the evicted destructors may unwind,
+                // and the slot must already hold a live value by then.
+                let evicted_key = self.keys[lru].as_ptr().read();
+                let evicted_value = self.values[lru].as_ptr().read();
 
-            self.keys[lru] = MaybeUninit::new(k);
-            self.values[lru] = MaybeUninit::new(value);
+                self.keys[lru] = MaybeUninit::new(k);
+                self.values[lru] = MaybeUninit::new(value);
+
+                // The line no longer owns them, so an unwind here is harmless.
+                drop((evicted_key, evicted_value));
 
-            unsafe { &*self.values[lru].as_ptr() }
+                &*self.values[lru].as_ptr()
+            }
         }
     }
 
@@ -694,3 +704,71 @@ impl<K: Eq + Hash, V, L: CacheLine<K, V>, H: Hasher + Default>
         Self::from_storage_and_hasher(buf, hash_builder)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{CacheLine, LruCache2, UnitCache};
+    use crate::test_utils::*;
+    use std::panic::{catch_unwind, AssertUnwindSafe};
+
+    /// `UnitCache::clear` must not leave the line claiming ownership of an
+    /// entry it has already destroyed.
+    ///
+    /// The method destroys the key, then the value, and clears the occupancy
+    /// flag only afterwards. A destructor is user-controlled and may unwind;
+    /// if it does, the flag stays set, and `Drop for UnitCache` destroys both
+    /// halves of the entry a second time.
+    #[test]
+    fn unit_cache_clear_is_panic_safe() {
+        let drop_count = DropCounter::new();
+
+        let result = catch_unwind(AssertUnwindSafe(|| {
+            let mut line = UnitCache::<usize, Droppable<usize>>::default();
+            line.insert(0, drop_count.new_droppable(0).panic_on_drop());
+
+            line.clear();
+        }));
+
+        assert!(
+            result.is_err(),
+            "the stored value's destructor must have unwound out of `clear`"
+        );
+        assert_eq!(
+            drop_count.dropped(),
+            1,
+            "the stored value must be destroyed exactly once"
+        );
+    }
+
+    /// `LruCache2::get_or_insert_with` must not leave the line claiming
+    /// ownership of an entry it has already destroyed.
+    ///
+    /// When the line is full, the method destroys the least recently used
+    /// key and value in place and only then writes the replacements. If the
+    /// evicted value's destructor unwinds, the slot is left uninitialized
+    /// while the state word still counts it as live, so `Drop for LruCache2`
+    /// destroys it a second time.
+    #[test]
+    fn lru_cache_2_get_or_insert_with_is_panic_safe() {
+        let drop_count = DropCounter::new();
+
+        let result = catch_unwind(AssertUnwindSafe(|| {
+            let mut line = LruCache2::<usize, Droppable<usize>>::default();
+            line.insert(0, drop_count.new_droppable(0).panic_on_drop());
+            line.insert(1, drop_count.new_droppable(1));
+
+            // The line is full, so this evicts one of the entries above.
+            line.get_or_insert_with(2, |_| drop_count.new_droppable(2));
+        }));
+
+        assert!(
+            result.is_err(),
+            "the evicted value's destructor must have unwound"
+        );
+        assert_eq!(
+            drop_count.dropped(),
+            3,
+            "each value must be destroyed exactly once"
+        );
+    }
+}