From f129313fb4204c604b7a1634d4396a9607b92443 Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 09:39:50 +0900 Subject: [PATCH 1/8] Fix panic-safety of Vec::truncate An element's destructor may unwind. truncate committed the new length only after destroying the tail, so an unwind left the vector claiming ownership of already-destroyed elements, which Drop for Vec then destroyed a second time. Commit the length first and destroy the tail as a slice, matching Drop for Vec and std's Vec::truncate. Destroying the tail as a slice also keeps the remaining elements from leaking when one of them unwinds. Adds an opt-in panic_on_drop to test_utils::Droppable, and pulls in std under cfg(test) for catch_unwind. --- src/collections/vec.rs | 57 +++++++++++++++++++++++++++++++++++++----- src/lib.rs | 23 +++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/collections/vec.rs b/src/collections/vec.rs index 4025a93..25512c6 100644 --- a/src/collections/vec.rs +++ b/src/collections/vec.rs @@ -422,13 +422,18 @@ impl>, I: Capacity> Vec { return; } - for i in new_len..old_len { - unsafe { - mut_ptr_at_index(&mut self.buf, i).drop_in_place(); - } - } - + // Commit the new length before destroying anything: an element's + // destructor may unwind, and the vector must not be left claiming + // ownership of values that have already been destroyed. self.len = len; + + unsafe { + let tail = ptr::slice_from_raw_parts_mut( + mut_ptr_at_index(&mut self.buf, new_len), + old_len - new_len, + ); + ptr::drop_in_place(tail); + } } /// Clears the vector, dropping all values. @@ -1910,4 +1915,44 @@ mod tests { } } } + + /// `Vec::truncate` must not leave the vector claiming ownership of + /// elements it has already destroyed. + /// + /// The method destroys the tail elements one by one and commits the new + /// length only afterwards. An element's destructor is user-controlled and + /// may unwind; if it does, the length is never updated, and `Drop for Vec` + /// destroys the whole `0..old_len` range a second time. + #[test] + fn truncate_is_panic_safe() { + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut backing_region = [ + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + ]; + + let mut vec = SliceVec::>::from(&mut backing_region[..]); + vec.push(drop_count.new_droppable(0)); + vec.push(drop_count.new_droppable(1).panic_on_drop()); + vec.push(drop_count.new_droppable(2)); + + vec.truncate(1); + })); + + assert!( + result.is_err(), + "the element at index 1 must have unwound out of `truncate`" + ); + assert_eq!( + drop_count.dropped(), + 3, + "each of the three elements must be destroyed exactly once" + ); + } } diff --git a/src/lib.rs b/src/lib.rs index e4aa374..3010989 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -214,6 +214,9 @@ impl CapacityError { /// This type definition is generally used to avoid writing out [`CapacityError`] directly and is otherwise a direct mapping to [`core::result::Result`]. pub type Result = core::result::Result; +#[cfg(test)] +extern crate std; + #[cfg(test)] mod test_utils { use core::cell::Cell; @@ -245,6 +248,7 @@ mod test_utils { pub(crate) fn new_droppable(&self, value: T) -> Droppable<'_, T> { Droppable { counter: self, + panic_on_drop: false, value, } } @@ -257,13 +261,32 @@ mod test_utils { #[derive(Debug)] pub(crate) struct Droppable<'a, T = ()> { counter: &'a DropCounter, + panic_on_drop: bool, pub value: T, } + impl<'a, T> Droppable<'a, T> { + /// Makes this value's destructor unwind after it has been counted. + /// + /// Used to check that a collection stays consistent when a + /// user-supplied destructor panics part-way through an operation. + pub(crate) fn panic_on_drop(mut self) -> Self { + self.panic_on_drop = true; + self + } + } + impl<'a, T> Drop for Droppable<'a, T> { fn drop(&mut self) { let new_drop_count = self.counter.drop_count.get() + 1; self.counter.drop_count.set(new_drop_count); + + if self.panic_on_drop { + // Disarm first: a panic escaping a destructor that is itself + // running during an unwind aborts the process. + self.panic_on_drop = false; + panic!("Droppable::drop panicked on purpose"); + } } } } From 846a3c2769852373de98f09811c74e6b6d76107e Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 10:03:38 +0900 Subject: [PATCH 2/8] Fix panic-safety of ListMap, PackedPool and cache lines These four methods destroy values in place and commit the metadata that removes them from the collection's logical state only afterwards. A destructor is user-controlled and may unwind, and for ListMap and PackedPool the Drop impl calls clear again, re-entering the same loop over stale metadata. ListMap::clear, PackedPool::clear: commit the emptied state first, then destroy the values as a slice so a panicking destructor does not leak the remaining ones. PackedPool recycles its slots in a separate loop beforehand, as that part cannot panic. ListMap::remove, LruCache2::get_or_insert_with, UnitCache::clear: move the removed values out and finish updating the collection before running their destructors, so an unwind cannot leave a slot both uninitialized and claimed. --- src/collections/cache.rs | 98 ++++++++++++++++++++++++--- src/collections/list_map.rs | 117 ++++++++++++++++++++++++++++++--- src/collections/pool/packed.rs | 51 +++++++++++++- 3 files changed, 244 insertions(+), 22 deletions(-) diff --git a/src/collections/cache.rs b/src/collections/cache.rs index bf46ef7..a203994 100644 --- a/src/collections/cache.rs +++ b/src/collections/cache.rs @@ -159,12 +159,16 @@ impl CacheLine for UnitCache { 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 CacheLine for LruCache2 { 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, 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::>::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::>::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" + ); + } +} diff --git a/src/collections/list_map.rs b/src/collections/list_map.rs index a0f0a94..12d97f2 100644 --- a/src/collections/list_map.rs +++ b/src/collections/list_map.rs @@ -318,16 +318,20 @@ impl>, I: Capacity> ListMap { /// assert!(map.is_empty()); /// ``` pub fn clear(&mut self) { + let old_len = self.len(); + + // Commit the new length before destroying anything: a destructor may + // unwind, and `Drop for ListMap` calls this method again. Without the + // early commit it would re-enter this loop over a stale length and + // destroy every entry a second time. + self.len = I::from_usize(0); + unsafe { let keys = self.buf.get_mut_ptr().cast::(); let values = self.buf.get_mut_ptr().add(self.values_offset()).cast::(); - for i in 0..self.len() { - keys.add(i).drop_in_place(); - values.add(i).drop_in_place(); - } - - self.len = I::from_usize(0); + core::ptr::drop_in_place(core::ptr::slice_from_raw_parts_mut(keys, old_len)); + core::ptr::drop_in_place(core::ptr::slice_from_raw_parts_mut(values, old_len)); } } @@ -590,11 +594,13 @@ impl>, I: Capacity> ListMap { unsafe { let buf_ptr = self.buf.get_mut_ptr(); - let keys = buf_ptr.cast::(); - keys.add(idx).drop_in_place(); - let values = buf_ptr.add(self.values_offset()).cast::(); + + // Move both halves of the entry out and back-fill the hole before + // running any user code: the key's destructor may unwind, and the + // map must already describe its post-removal state by then. + let removed_key = keys.add(idx).read(); let result = values.add(idx).read(); if idx != new_len { @@ -603,6 +609,10 @@ impl>, I: Capacity> ListMap { } self.len = I::from_usize(new_len); + + // The map no longer owns the key, so an unwind here is harmless. + drop(removed_key); + Some(result) } } @@ -1723,3 +1733,92 @@ where self.for_each(drop); } } + +#[cfg(test)] +mod tests { + use crate::collections::InlineListMap; + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + /// A key that borrows as `usize` so `remove` can be called with a plain + /// index, while the key itself carries a destructor that can unwind. + #[derive(Debug)] + struct Key<'a>(Droppable<'a, usize>); + + impl core::borrow::Borrow for Key<'_> { + fn borrow(&self) -> &usize { + &self.0.value + } + } + + impl PartialEq for Key<'_> { + fn eq(&self, other: &Self) -> bool { + self.0.value == other.0.value + } + } + + impl Eq for Key<'_> {} + + /// `ListMap::clear` must not leave the map claiming ownership of entries + /// it has already destroyed. + /// + /// The method destroys every key and value and zeroes the length only + /// afterwards. A value's destructor is user-controlled and may unwind; if + /// it does, the length is never updated, and `Drop for ListMap` calls + /// `clear` again, re-entering the same loop over a stale length. + #[test] + fn clear_is_panic_safe() { + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut map = InlineListMap::, 4>::new(); + map.insert(0, drop_count.new_droppable(0)); + map.insert(1, drop_count.new_droppable(1).panic_on_drop()); + map.insert(2, drop_count.new_droppable(2)); + + map.clear(); + })); + + assert!( + result.is_err(), + "the value at index 1 must have unwound out of `clear`" + ); + assert_eq!( + drop_count.dropped(), + 3, + "each stored value must be destroyed exactly once" + ); + } + + /// `ListMap::remove` must not leave the map claiming ownership of the key + /// it has already destroyed. + /// + /// The method destroys the key at the removed index, reads the value out, + /// back-fills the hole from the last entry, and decrements the length only + /// afterwards. If the key's destructor unwinds, the length still counts + /// the removed entry, and `Drop for ListMap` destroys that key a second + /// time. + #[test] + fn remove_is_panic_safe() { + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut map = InlineListMap::, usize, 4>::new(); + map.insert(Key(drop_count.new_droppable(0)), 0); + map.insert(Key(drop_count.new_droppable(1).panic_on_drop()), 1); + map.insert(Key(drop_count.new_droppable(2)), 2); + + map.remove(&1usize); + })); + + assert!( + result.is_err(), + "the removed key's destructor must have unwound" + ); + assert_eq!( + drop_count.dropped(), + 3, + "each stored key must be destroyed exactly once" + ); + } +} diff --git a/src/collections/pool/packed.rs b/src/collections/pool/packed.rs index 1484515..9e45615 100644 --- a/src/collections/pool/packed.rs +++ b/src/collections/pool/packed.rs @@ -650,10 +650,14 @@ impl>, H: Handle> PackedPool { /// assert!(pool.is_empty()); /// ``` pub fn clear(&mut self) { - for packed_index in 0..self.len() { - unsafe { - self.values_mut_ptr().add(packed_index).drop_in_place(); + let old_len = self.len(); + // Recycle every slot before destroying anything: a value's destructor + // may unwind, and `Drop for PackedPool` calls this method again. The + // pool must already describe its emptied state by then, or the loop is + // re-entered over a stale length and every value is destroyed twice. + for packed_index in 0..old_len { + unsafe { let (index, _) = self.handles_ptr().add(packed_index).read().into_raw_parts(); *self.counters_mut().add(index) += 1; @@ -664,6 +668,11 @@ impl>, H: Handle> PackedPool { } self.len = H::Index::from_usize(0); + + unsafe { + let values = self.values_mut_ptr(); + core::ptr::drop_in_place(core::ptr::slice_from_raw_parts_mut(values, old_len)); + } } /// Creates an iterator visiting all handle-value pairs in arbitrary order, @@ -1295,4 +1304,40 @@ mod tests { test_layout::(); test_layout::, DefaultHandle, 80>(); } + + /// `PackedPool::clear` must not leave the pool claiming ownership of + /// values it has already destroyed. + /// + /// The method destroys each value and recycles its slot in the same loop, + /// committing the new length only afterwards. A value's destructor is + /// user-controlled and may unwind; if it does, the length is never + /// updated, and `Drop for PackedPool` calls `clear` again, re-entering the + /// same loop over a stale length. + #[test] + fn clear_is_panic_safe() { + use crate::collections::PackedInlinePool; + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut pool = PackedInlinePool::, 4>::new(); + pool.insert(drop_count.new_droppable(0)); + pool.insert(drop_count.new_droppable(1).panic_on_drop()); + pool.insert(drop_count.new_droppable(2)); + + pool.clear(); + })); + + assert!( + result.is_err(), + "the value at packed index 1 must have unwound out of `clear`" + ); + assert_eq!( + drop_count.dropped(), + 3, + "each stored value must be destroyed exactly once" + ); + } } From 8ad2ac85a8b0c108f797e157e4ff9de4769705c9 Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 10:24:21 +0900 Subject: [PATCH 3/8] Fix panic-safety of DirectPool::try_insert_with_handle The method bumped the length and made the slot's generation counter odd -- the pool's encoding of an occupied slot -- before calling the user-supplied closure that produces the value. If the closure unwound, the slot stayed uninitialized while the pool still claimed it, and Drop for DirectPool destroyed that uninitialized memory. Run the closure first and update the pool only once the value exists. The free-list link is read out of the slot union before the value is written over it. Without the fix the added test terminates the process with SIGSEGV. --- src/collections/pool/direct.rs | 48 +++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/src/collections/pool/direct.rs b/src/collections/pool/direct.rs index b431f6b..09806fa 100644 --- a/src/collections/pool/direct.rs +++ b/src/collections/pool/direct.rs @@ -327,18 +327,25 @@ impl>, H: Handle> DirectPool { return None; } - self.len = H::Index::from_usize(self.len() + 1); unsafe { let gen_count_ptr = self.gen_counts_mut().add(insert_position); let gen_count = gen_count_ptr.read().wrapping_add(1) & H::MAX_GENERATION; debug_assert_eq!(gen_count % 2, 1); - gen_count_ptr.write(gen_count); + + let handle = H::new(insert_position, gen_count); + + // Run the user-supplied closure before touching the pool: it may + // unwind, and marking the slot occupied first would leave the pool + // claiming ownership of memory that was never initialized. + let item = f(handle); let slot = self.slots_mut().add(insert_position); self.next_free_slot = (*slot).next_free_slot; - let handle = H::new(insert_position, gen_count); + (*slot).item = ManuallyDrop::new(item); + + gen_count_ptr.write(gen_count); + self.len = H::Index::from_usize(self.len() + 1); - (*slot).item = ManuallyDrop::new(f(handle)); Some(handle) } } @@ -1311,4 +1318,37 @@ mod tests { pool.drain(); assert_eq!(drop_count.dropped() as u64, inserted); } + + /// `DirectPool::try_insert_with_handle` must not mark a slot as occupied + /// before the value that fills it exists. + /// + /// The method bumps the length and makes the slot's generation counter odd + /// — the pool's encoding of "this slot holds a live value" — before calling + /// the user-supplied closure. If the closure unwinds, the slot is left + /// uninitialized while the pool still claims it, and `Drop for DirectPool` + /// destroys that uninitialized memory. + #[test] + fn try_insert_with_handle_is_panic_safe() { + use crate::collections::DirectInlinePool; + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut pool = DirectInlinePool::, 4>::new(); + pool.insert(drop_count.new_droppable(0)); + + pool.insert_with_handle(|_| -> Droppable<'_, usize> { + panic!("the filler closure unwound"); + }); + })); + + assert!(result.is_err(), "the filler closure must have unwound"); + assert_eq!( + drop_count.dropped(), + 1, + "only the one value that was actually stored may be destroyed" + ); + } } From 60ce92afe44aabea6008265d2cecdf785c301b89 Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 13:07:18 +0900 Subject: [PATCH 4/8] Fix Deque::truncate and Deque::retain Both methods addressed the ring buffer with a bare 'i % capacity', ignoring the 'front' offset that physical_index applies everywhere else. With a non-zero front -- one push_front is enough -- they destroyed slots that were never initialized, passed uninitialized memory to the user's predicate, and left live elements untouched. No panic is needed to reach this. Both also committed the new length only after destroying or compacting, so an unwinding destructor or predicate left the deque claiming values it no longer owned. truncate now translates logical indices through front, commits the length first, and drops the removed range as one or two slices so a panicking destructor does not leak the elements behind it. retain translates its source and destination indices through front and keeps its progress in a drop guard, which compacts the unvisited tail back into the deque when the predicate or a destructor unwinds. --- src/collections/deque.rs | 240 +++++++++++++++++++++++++++++++++++---- 1 file changed, 219 insertions(+), 21 deletions(-) diff --git a/src/collections/deque.rs b/src/collections/deque.rs index 41e8230..d2fd626 100644 --- a/src/collections/deque.rs +++ b/src/collections/deque.rs @@ -424,15 +424,29 @@ impl>, I: Capacity> Deque { return; } - for i in new_len..old_len { - let idx = i % self.capacity(); - let ptr = self.buf.get_mut_ptr().cast::(); - unsafe { - ptr.add(idx).drop_in_place(); - } - } + let capacity = self.capacity(); + // Logical indices are offset by `front`; see `physical_index`. + let start = (self.front.as_usize() + new_len) % capacity; + let count = old_len - new_len; + // Commit the new length before destroying anything: an element's + // destructor may unwind, and the deque must not be left claiming + // ownership of values that have already been destroyed. self.len = len; + + unsafe { + let base = self.buf.get_mut_ptr().cast::(); + + // The removed range may wrap around the end of the buffer. Drop + // each contiguous run as a slice, so that a panicking destructor + // does not leak the elements that follow it. + let head = count.min(capacity - start); + core::ptr::drop_in_place(core::ptr::slice_from_raw_parts_mut(base.add(start), head)); + + if head < count { + core::ptr::drop_in_place(core::ptr::slice_from_raw_parts_mut(base, count - head)); + } + } } /// Clears the `Deque`, dropping all values. @@ -840,29 +854,77 @@ impl>, I: Capacity> Deque { F: FnMut(&T) -> bool, { let capacity = self.capacity(); + let front = self.front.as_usize(); let old_len = self.len(); - let mut new_len = 0; - for i in 0..old_len { - let idx = i % capacity; - let src = ptr_at_index(&self.buf, idx); - let retain = f(unsafe { &*src }); + /// Restores the deque to a consistent state when `f` or a destructor + /// unwinds: the elements that were kept stay at the front, and the + /// ones that have not been visited yet are moved up behind them so + /// that they are neither leaked nor destroyed twice. + struct Guard<'a, T, S: Storage>, I: Capacity> { + deque: &'a mut Deque, + visited: usize, + kept: usize, + old_len: usize, + } - if retain { - let dst = mut_ptr_at_index(&mut self.buf, new_len % capacity); - unsafe { - core::ptr::copy(src, dst, 1); + impl>, I: Capacity> Drop for Guard<'_, T, S, I> { + fn drop(&mut self) { + let capacity = self.deque.capacity(); + let front = self.deque.front.as_usize(); + + // Compact the unvisited tail up behind the kept elements. + for i in self.visited..self.old_len { + let src = (front + i) % capacity; + let dst = (front + self.kept) % capacity; + if src != dst { + unsafe { + let base = self.deque.buf.get_mut_ptr().cast::(); + core::ptr::copy_nonoverlapping(base.add(src), base.add(dst), 1); + } + } + self.kept += 1; } - new_len += 1; + + self.deque.len = I::from_usize(self.kept); + } + } + + let mut guard = Guard { + deque: self, + visited: 0, + kept: 0, + old_len, + }; + + for i in 0..old_len { + // Logical indices are offset by `front`; see `physical_index`. + let idx = (front + i) % capacity; + let src = ptr_at_index(&guard.deque.buf, idx); + + // `f` may unwind. Until it returns, this element is neither kept + // nor removed, so the guard must still account for it. + if f(unsafe { &*src }) { + let dst_idx = (front + guard.kept) % capacity; + if dst_idx != idx { + unsafe { + let base = guard.deque.buf.get_mut_ptr().cast::(); + core::ptr::copy_nonoverlapping(base.add(idx), base.add(dst_idx), 1); + } + } + guard.kept += 1; + guard.visited = i + 1; } else { - let to_drop = mut_ptr_at_index(&mut self.buf, idx); + // Mark the element as consumed before destroying it: its + // destructor may unwind, and the guard must not move a slot + // that no longer holds a live value. + guard.visited = i + 1; unsafe { - core::ptr::drop_in_place(to_drop); + let base = guard.deque.buf.get_mut_ptr().cast::(); + core::ptr::drop_in_place(base.add(idx)); } } } - - self.len = I::from_usize(new_len); } fn rotate_left_inner(&mut self, mid: usize) { @@ -2076,4 +2138,140 @@ mod tests { } } } + + /// Probe: does `truncate` account for the deque's `front` offset? + #[test] + fn truncate_respects_front() { + use crate::test_utils::*; + + let drop_count = DropCounter::new(); + + { + let mut backing_region = [ + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + ]; + + let mut deque = + crate::collections::SliceDeque::>::from(&mut backing_region[..]); + // Wraps the front around to the end of the buffer. + deque.push_front(drop_count.new_droppable(0)); + deque.push_back(drop_count.new_droppable(1)); + assert_eq!(deque.len(), 2); + + deque.truncate(1); + assert_eq!(deque.len(), 1); + assert_eq!(deque[0].value, 0, "the front element must survive"); + } + + assert_eq!( + drop_count.dropped(), + 2, + "each element must be destroyed exactly once" + ); + } + + /// Probe: does `retain` account for the deque's `front` offset? + #[test] + fn retain_respects_front() { + let mut backing_region = [core::mem::MaybeUninit::::uninit(); 4]; + let mut deque = crate::collections::SliceDeque::::from(&mut backing_region[..]); + + // Wraps the front around to the end of the buffer. + deque.push_front(10); + deque.push_back(20); + deque.push_back(30); + assert_eq!(deque.as_slices(), (&[10][..], &[20, 30][..])); + + let mut seen = std::vec::Vec::new(); + deque.retain(|&x| { + seen.push(x); + x != 10 + }); + + assert_eq!(seen, [10, 20, 30], "the predicate must see every element, once"); + assert_eq!(deque.len(), 2); + assert_eq!(deque.get(0), Some(&20)); + assert_eq!(deque.get(1), Some(&30)); + } + + /// `Deque::truncate` must not leave the deque claiming ownership of + /// elements it has already destroyed. + #[test] + fn truncate_is_panic_safe() { + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut backing_region = [ + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + ]; + + let mut deque = + crate::collections::SliceDeque::>::from(&mut backing_region[..]); + deque.push_front(drop_count.new_droppable(0)); + deque.push_back(drop_count.new_droppable(1).panic_on_drop()); + deque.push_back(drop_count.new_droppable(2)); + + deque.truncate(1); + })); + + assert!( + result.is_err(), + "the element at index 1 must have unwound out of `truncate`" + ); + assert_eq!( + drop_count.dropped(), + 3, + "each element must be destroyed exactly once" + ); + } + + /// `Deque::retain` must not leave the deque claiming ownership of an + /// element that has been destroyed, or of a slot that a compaction has + /// duplicated. + #[test] + fn retain_is_panic_safe() { + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_count = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut backing_region = [ + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + core::mem::MaybeUninit::>::uninit(), + ]; + + let mut deque = + crate::collections::SliceDeque::>::from(&mut backing_region[..]); + deque.push_front(drop_count.new_droppable(0)); + deque.push_back(drop_count.new_droppable(1)); + deque.push_back(drop_count.new_droppable(2)); + + // No destructor needs to panic: the predicate alone is enough. + // By the time it unwinds, element 0 has been compacted, so the + // value exists in two slots. + deque.retain(|x| { + assert!(x.value < 2, "the predicate unwound"); + true + }); + })); + + assert!(result.is_err(), "the predicate must have unwound"); + assert_eq!( + drop_count.dropped(), + 3, + "each element must be destroyed exactly once" + ); + } } From b5138a52943d0ccc4a0b9604f17dbc4d005d4448 Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 16:41:01 +0900 Subject: [PATCH 5/8] Make the Deque::retain regression test exercise a compaction The previous version pushed to the front, which meant the indexing bug diverted the traversal before any element was compacted -- the test passed against the unfixed code. Keep the front at zero so the predicate unwinds after two elements have been copied forward, which is the state the fix is about. --- src/collections/deque.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/collections/deque.rs b/src/collections/deque.rs index d2fd626..02a488a 100644 --- a/src/collections/deque.rs +++ b/src/collections/deque.rs @@ -2254,23 +2254,24 @@ mod tests { let mut deque = crate::collections::SliceDeque::>::from(&mut backing_region[..]); - deque.push_front(drop_count.new_droppable(0)); + deque.push_back(drop_count.new_droppable(0)); deque.push_back(drop_count.new_droppable(1)); deque.push_back(drop_count.new_droppable(2)); + deque.push_back(drop_count.new_droppable(3)); // No destructor needs to panic: the predicate alone is enough. - // By the time it unwinds, element 0 has been compacted, so the - // value exists in two slots. + // Element 0 is dropped, so elements 1 and 2 are compacted one slot + // towards the front before the predicate unwinds on element 3. deque.retain(|x| { - assert!(x.value < 2, "the predicate unwound"); - true + assert!(x.value != 3, "the predicate unwound"); + x.value != 0 }); })); assert!(result.is_err(), "the predicate must have unwound"); assert_eq!( drop_count.dropped(), - 3, + 4, "each element must be destroyed exactly once" ); } From 98519b4005225bc9751b207f6bf5b05982b475b6 Mon Sep 17 00:00:00 2001 From: tooson Date: Fri, 28 Aug 2026 16:52:02 +0900 Subject: [PATCH 6/8] Update CHANGELOG --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 500cd67..d8c474b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ ## 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` 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 From e5d9d611f4d5de8130c3511da8e20f5a2e179f2b Mon Sep 17 00:00:00 2001 From: tooson Date: Mon, 14 Sep 2026 17:26:14 +0900 Subject: [PATCH 7/8] Make OptionGroup::clear panic-safe `clear` destroys every `Some` value and clears the flag word only afterwards. A destructor may unwind, and when it does the flags stay set, so `Drop for OptionGroup` runs `drop_all_in_place` again over the same bits. Retiring the flags first makes the second pass a no-op. Values not yet reached when the unwind starts are leaked, which is safe. Adds `clear_is_panic_safe`: without this change a two-element group reports 4 drops for 2 values. --- src/collections/option_group.rs | 42 +++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/collections/option_group.rs b/src/collections/option_group.rs index 1919bc2..5b77833 100644 --- a/src/collections/option_group.rs +++ b/src/collections/option_group.rs @@ -288,10 +288,15 @@ where /// Sets all `Some` values in the group to `None`. pub fn clear(&mut self) { + // Retire the flags before destroying. `drop_all_in_place` runs each + // value's `Drop`, which may unwind; if `self.flags` were cleared + // afterwards it would still be set, and `Drop for OptionGroup` would + // run `drop_all_in_place` again over the same bits. + let flags = self.flags; + self.flags = F::ZERO; unsafe { - T::drop_all_in_place(&mut self.value, self.flags.into()); + T::drop_all_in_place(&mut self.value, flags.into()); } - self.flags = F::ZERO; } } @@ -1414,4 +1419,37 @@ mod test { drop(option_array); assert_eq!(drop_counter.dropped(), 6); } + + /// `OptionGroup::clear` must not leave the group claiming ownership of + /// values it has already destroyed. + /// + /// The method destroys every `Some` value and clears the flag word only + /// afterwards. A destructor is user-controlled and may unwind; if it does, + /// the flags stay set, and `Drop for OptionGroup` runs `drop_all_in_place` + /// a second time over the same bits. + #[test] + fn clear_is_panic_safe() { + use crate::test_utils::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + + let drop_counter = DropCounter::new(); + + let result = catch_unwind(AssertUnwindSafe(|| { + let mut option_tuple: OptionGroup8<(Droppable, Droppable)> = OptionGroup8::empty(); + option_tuple.insert_0(drop_counter.new_droppable(())); + option_tuple.insert_1(drop_counter.new_droppable(()).panic_on_drop()); + + option_tuple.clear(); + })); + + assert!( + result.is_err(), + "the stored value's destructor must have unwound out of `clear`" + ); + assert_eq!( + drop_counter.dropped(), + 2, + "each stored value must be destroyed exactly once" + ); + } } From cb04390c106d8455b38e296fb02be5330aa53896 Mon Sep 17 00:00:00 2001 From: tooson Date: Mon, 14 Sep 2026 17:27:45 +0900 Subject: [PATCH 8/8] Update CHANGELOG --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8c474b..eb3b0a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,8 @@ the front. - Make `Vec::{truncate, clear}`, `Deque::{truncate, clear, retain}`, `ListMap::{clear, remove}`, `PackedPool::clear`, `UnitCache::clear`, - `LruCache2::get_or_insert_with` and `DirectPool::try_insert_with_handle` + `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. @@ -76,4 +77,4 @@ # 0.1.0 (2020-12-03) -Initial Release \ No newline at end of file +Initial Release