From cbaf9e28af922e42eea91abd7da66a40c650ddc1 Mon Sep 17 00:00:00 2001 From: tooson Date: Thu, 27 Aug 2026 13:42:25 +0900 Subject: [PATCH 1/3] Fix double free when retain unwinds retain compacts kept elements with copy_nonoverlapping and commits the new length only after the loop. Both the predicate and a removed element's Drop are user code, so an unwind leaves either a duplicated slot or an already-dropped one inside 0..len, and StackVec::drop destroys it again. Use a backshift guard that restores a correct length on every exit path, matching what Vec::retain does. --- src/stack_vec.rs | 147 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 132 insertions(+), 15 deletions(-) diff --git a/src/stack_vec.rs b/src/stack_vec.rs index 03be4ea..d630d8d 100644 --- a/src/stack_vec.rs +++ b/src/stack_vec.rs @@ -461,34 +461,71 @@ impl StackVec { where F: FnMut(&mut T) -> bool, { - let base_ptr = self.as_mut_ptr(); - let mut kept = 0; + // Both the predicate and a removed element's `Drop` are user code and + // may unwind. Retire the whole buffer up front and let a guard restore + // a correct length on every exit path, so a duplicated or + // already-dropped slot can never stay inside `0..len`. + let original_len = self.len; + self.len = 0; + + struct BackshiftOnDrop<'vec, T, const CAP: usize> { + vec: &'vec mut StackVec, + processed: usize, + deleted: usize, + original_len: usize, + } + + impl Drop for BackshiftOnDrop<'_, T, CAP> { + fn drop(&mut self) { + if self.deleted > 0 { + // SAFETY: the unprocessed tail is still initialized and the + // destination range is inside the buffer. + unsafe { + ptr::copy( + self.vec.as_ptr().add(self.processed), + self.vec.as_mut_ptr().add(self.processed - self.deleted), + self.original_len - self.processed, + ); + } + } + self.vec.len = self.original_len - self.deleted; + } + } - for i in 0..self.len { - // SAFETY: i < self.len, so this is a valid and initialized element - let ptr = unsafe { base_ptr.add(i) }; - // SAFETY: ptr points to valid initialized element + let mut g = BackshiftOnDrop { + vec: self, + processed: 0, + deleted: 0, + original_len, + }; + + while g.processed < original_len { + // SAFETY: processed < original_len, so this element is initialized. + let ptr = unsafe { g.vec.as_mut_ptr().add(g.processed) }; + // SAFETY: ptr points to a valid initialized element. let elem = unsafe { &mut *ptr }; if f(elem) { - if kept != i { - // SAFETY: kept < i < self.len, ranges don't overlap - let dst = unsafe { base_ptr.add(kept) }; - // SAFETY: Copying single element from valid source to valid dest + if g.deleted > 0 { + // SAFETY: the destination is an earlier, already-vacated slot. unsafe { + let dst = g.vec.as_mut_ptr().add(g.processed - g.deleted); ptr::copy_nonoverlapping(ptr, dst, 1); } } - kept += 1; + g.processed += 1; } else { - // SAFETY: Dropping initialized element that won't be kept + // Count the slot out before destroying it, so an unwinding + // `Drop` leaves the guard with a consistent view. + g.processed += 1; + g.deleted += 1; + // SAFETY: dropping an initialized element that won't be kept. unsafe { - ptr::drop_in_place(elem); + ptr::drop_in_place(ptr); } } } - - self.len = kept; + // `g`'s `Drop` commits the final length. } // index @@ -1837,4 +1874,84 @@ mod tests { drop(iter); assert_eq!(counter.load(Ordering::SeqCst), 3); } + + /// A panic in `retain` must not leave a duplicated or already-destroyed + /// slot inside `0..len`. + #[cfg(feature = "std")] + #[test] + fn retain_panicking_predicate() { + extern crate std; + use std::panic::{AssertUnwindSafe, catch_unwind}; + + struct DropCounter(i32, Arc); + impl Drop for DropCounter { + fn drop(&mut self) { + self.1.fetch_add(1, Ordering::SeqCst); + } + } + + let counter = Arc::new(AtomicUsize::new(0)); + let mut vec = StackVec::::new(); + for i in 0..4_i32 { + vec.push(DropCounter(i, Arc::clone(&counter))); + } + + // Reject element 0 so the next kept one is compacted forward, then + // unwind on element 2 before the length is committed. + let r = catch_unwind(AssertUnwindSafe(|| { + vec.retain(|e| { + assert_ne!(e.0, 2_i32, "predicate panics"); + e.0 != 0_i32 + }); + })); + assert!(r.is_err(), "the predicate should have panicked"); + + drop(vec); + + // Four elements exist. Fewer drops mean a leak, which is sound; more + // mean a value was destroyed twice. + let drops = counter.load(Ordering::SeqCst); + assert!(drops <= 4, "{drops} drops for 4 elements"); + } + + /// The same, with the panic coming from a removed element's `Drop`. + #[cfg(feature = "std")] + #[test] + fn retain_panicking_element_drop() { + extern crate std; + use core::cell::Cell; + use std::panic::{AssertUnwindSafe, catch_unwind}; + + std::thread_local! { + static ARMED: Cell = const { Cell::new(false) }; + } + + struct Boom(i32, Arc); + impl Drop for Boom { + fn drop(&mut self) { + self.1.fetch_add(1, Ordering::SeqCst); + if ARMED.with(|a| a.replace(false)) { + panic!("element Drop panics"); + } + } + } + + let counter = Arc::new(AtomicUsize::new(0)); + let mut vec = StackVec::::new(); + for i in 0..4_i32 { + vec.push(Boom(i, Arc::clone(&counter))); + } + + ARMED.with(|a| a.set(true)); + let r = catch_unwind(AssertUnwindSafe(|| { + vec.retain(|e| e.0 != 1_i32); + })); + ARMED.with(|a| a.set(false)); + assert!(r.is_err(), "the armed Drop should have panicked"); + + drop(vec); + + let drops = counter.load(Ordering::SeqCst); + assert!(drops <= 4, "{drops} drops for 4 elements"); + } } From c15f4b5b024b7017e2d12fc52c6de532d788100b Mon Sep 17 00:00:00 2001 From: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:36:48 +0100 Subject: [PATCH 2/3] fixup: resolve Miri aliasing violation Signed-off-by: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com> --- src/stack_vec.rs | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/stack_vec.rs b/src/stack_vec.rs index d630d8d..a2637bf 100644 --- a/src/stack_vec.rs +++ b/src/stack_vec.rs @@ -478,12 +478,16 @@ impl StackVec { impl Drop for BackshiftOnDrop<'_, T, CAP> { fn drop(&mut self) { if self.deleted > 0 { + let base = self.vec.as_mut_ptr(); // SAFETY: the unprocessed tail is still initialized and the - // destination range is inside the buffer. + // destination range is inside the buffer. Deriving + // both pointers from a single reborrow avoids + // invalidating an earlier shared reborrow under + // Stacked Borrows. unsafe { ptr::copy( - self.vec.as_ptr().add(self.processed), - self.vec.as_mut_ptr().add(self.processed - self.deleted), + base.add(self.processed), + base.add(self.processed - self.deleted), self.original_len - self.processed, ); } @@ -500,16 +504,21 @@ impl StackVec { }; while g.processed < original_len { + let base = g.vec.as_mut_ptr(); + // SAFETY: processed < original_len, so this element is initialized. - let ptr = unsafe { g.vec.as_mut_ptr().add(g.processed) }; + let ptr = unsafe { base.add(g.processed) }; // SAFETY: ptr points to a valid initialized element. let elem = unsafe { &mut *ptr }; if f(elem) { if g.deleted > 0 { - // SAFETY: the destination is an earlier, already-vacated slot. + // SAFETY: dst is an earlier, already-vacated slot; ptr and dst + // both derive from the same reborrow, so neither + // invalidates the other. + let dst = unsafe { base.add(g.processed - g.deleted) }; + // SAFETY: copying a single initialized element to a vacated slot. unsafe { - let dst = g.vec.as_mut_ptr().add(g.processed - g.deleted); ptr::copy_nonoverlapping(ptr, dst, 1); } } @@ -1881,7 +1890,8 @@ mod tests { #[test] fn retain_panicking_predicate() { extern crate std; - use std::panic::{AssertUnwindSafe, catch_unwind}; + use core::panic::AssertUnwindSafe; + use std::panic::catch_unwind; struct DropCounter(i32, Arc); impl Drop for DropCounter { @@ -1920,7 +1930,8 @@ mod tests { fn retain_panicking_element_drop() { extern crate std; use core::cell::Cell; - use std::panic::{AssertUnwindSafe, catch_unwind}; + use core::panic::AssertUnwindSafe; + use std::panic::catch_unwind; std::thread_local! { static ARMED: Cell = const { Cell::new(false) }; @@ -1930,9 +1941,7 @@ mod tests { impl Drop for Boom { fn drop(&mut self) { self.1.fetch_add(1, Ordering::SeqCst); - if ARMED.with(|a| a.replace(false)) { - panic!("element Drop panics"); - } + assert!(!ARMED.replace(false), "element Drop panics"); } } From 229383cbdb7969e53347ce803c162ed311461749 Mon Sep 17 00:00:00 2001 From: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:39:48 +0100 Subject: [PATCH 3/3] chore: clippy Signed-off-by: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com> --- Cargo.toml | 2 -- src/stack_vec.rs | 38 +++++++++++++++++++------------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a838593..7c4e83c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,9 +5,7 @@ edition = "2024" authors = ["Kyle Thapar "] description = "Stack-allocated collections for Rust" license = "MIT OR Apache-2.0" -readme = "README.md" repository = "https://github.com/OverwrittenCode/stack_collections" -homepage = "https://github.com/OverwrittenCode/stack_collections" documentation = "https://docs.rs/stack_collections" keywords = ["stack", "collections", "no-heap"] categories = ["data-structures", "no-std", "no-std::no-alloc"] diff --git a/src/stack_vec.rs b/src/stack_vec.rs index a2637bf..64bc838 100644 --- a/src/stack_vec.rs +++ b/src/stack_vec.rs @@ -461,13 +461,6 @@ impl StackVec { where F: FnMut(&mut T) -> bool, { - // Both the predicate and a removed element's `Drop` are user code and - // may unwind. Retire the whole buffer up front and let a guard restore - // a correct length on every exit path, so a duplicated or - // already-dropped slot can never stay inside `0..len`. - let original_len = self.len; - self.len = 0; - struct BackshiftOnDrop<'vec, T, const CAP: usize> { vec: &'vec mut StackVec, processed: usize, @@ -479,23 +472,30 @@ impl StackVec { fn drop(&mut self) { if self.deleted > 0 { let base = self.vec.as_mut_ptr(); - // SAFETY: the unprocessed tail is still initialized and the - // destination range is inside the buffer. Deriving - // both pointers from a single reborrow avoids - // invalidating an earlier shared reborrow under - // Stacked Borrows. + // SAFETY: processed and processed - deleted are both + // within bounds of the buffer. + let src = unsafe { base.add(self.processed) }; + // SAFETY: see above. + let dst = unsafe { base.add(self.processed - self.deleted) }; + // SAFETY: the unprocessed tail is still initialized and + // the destination range is inside the buffer. + // src/dst derive from a single reborrow, so + // neither invalidates the other. unsafe { - ptr::copy( - base.add(self.processed), - base.add(self.processed - self.deleted), - self.original_len - self.processed, - ); + ptr::copy(src, dst, self.original_len - self.processed); } } self.vec.len = self.original_len - self.deleted; } } + // Both the predicate and a removed element's `Drop` are user code and + // may unwind. Retire the whole buffer up front and let a guard restore + // a correct length on every exit path, so a duplicated or + // already-dropped slot can never stay inside `0..len`. + let original_len = self.len; + self.len = 0; + let mut g = BackshiftOnDrop { vec: self, processed: 0, @@ -1951,11 +1951,11 @@ mod tests { vec.push(Boom(i, Arc::clone(&counter))); } - ARMED.with(|a| a.set(true)); + ARMED.set(true); let r = catch_unwind(AssertUnwindSafe(|| { vec.retain(|e| e.0 != 1_i32); })); - ARMED.with(|a| a.set(false)); + ARMED.set(false); assert!(r.is_err(), "the armed Drop should have panicked"); drop(vec);