diff --git a/CHANGELOG.md b/CHANGELOG.md index 500cd67..eb3b0a7 100644 --- a/CHANGELOG.md +++ b/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 \ No newline at end of file +Initial Release 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/deque.rs b/src/collections/deque.rs index 41e8230..02a488a 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,141 @@ 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_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. + // 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 != 3, "the predicate unwound"); + x.value != 0 + }); + })); + + assert!(result.is_err(), "the predicate must have unwound"); + assert_eq!( + drop_count.dropped(), + 4, + "each element 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/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" + ); + } } 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" + ); + } } 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" + ); + } } 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"); + } } } }