diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e640b4952..53b78e9b35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +- Fixed unsoundness in `Vec::IntoIter::drop` and `HistoryBuf::write`in the context of panicking drop implementations. + ## [v0.9.3] 2025-04-15 -- Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear` and `IndexMap::clear` in the context -of panicking drop implementations. +- Fixed unsoundness in `Deque::clear`, `HistoryBuf::clear` and `IndexMap::clear` in the context of panicking drop implementations. - Added `from_bytes_truncating_at_nul` to `CString` - Added `CString::{into_bytes, into_bytes_with_nul, into_string}` - Added `pop_front_if` and `pop_back_if` to `Deque` diff --git a/src/history_buf.rs b/src/history_buf.rs index 0d008143d7..efc3e4a717 100644 --- a/src/history_buf.rs +++ b/src/history_buf.rs @@ -385,9 +385,12 @@ impl + ?Sized> HistoryBufInner { /// Writes an element to the buffer, overwriting the oldest value. pub fn write(&mut self, t: T) { + let _tmp; if self.filled { - // Drop the old before we overwrite it. - unsafe { ptr::drop_in_place(self.data.borrow_mut()[self.write_at].as_mut_ptr()) } + // Copy the old so that it is dropped at the end + // We don't drop it now so that a panic in its destructor doesn't + // lead to an invalid state + _tmp = unsafe { ptr::read(self.data.borrow_mut()[self.write_at].as_mut_ptr()) }; } self.data.borrow_mut()[self.write_at] = MaybeUninit::new(t); @@ -994,10 +997,11 @@ mod tests { } } + // Tests that a panic in in a downstream drop implementation does not lead to an + // inconsistent state during unwinding that could lead to undefined behaviour. + // See https://github.com/rust-embedded/heapless/issues/646 #[test] fn test_use_after_free_clear() { - // See https://github.com/rust-embedded/heapless/issues/646 - static COUNT: AtomicI32 = AtomicI32::new(0); #[derive(Debug)] @@ -1032,6 +1036,45 @@ mod tests { assert_eq!(Dropper::count(), 0); } + // Tests that a panic in in a downstream drop implementation does not lead to an + // inconsistent state during unwinding that could lead to undefined behaviour. + // See https://github.com/rust-embedded/heapless/issues/659 + #[test] + fn test_use_after_free_write() { + static COUNT: AtomicI32 = AtomicI32::new(0); + + #[derive(Debug)] + struct Dropper(bool); + + impl Dropper { + fn new(should_panic: bool) -> Self { + COUNT.fetch_add(1, Ordering::Relaxed); + Self(should_panic) + } + fn count() -> i32 { + COUNT.load(Ordering::Relaxed) + } + } + impl Drop for Dropper { + fn drop(&mut self) { + COUNT.fetch_sub(1, Ordering::Relaxed); + assert!(!self.0, "Testing panicking"); + } + } + + let mut histbuf = HistoryBuf::::new(); + histbuf.write(Dropper::new(true)); + histbuf.write(Dropper::new(false)); + histbuf.write(Dropper::new(false)); + histbuf.write(Dropper::new(false)); + histbuf.write(Dropper::new(false)); + let mut unwind_safe = AssertUnwindSafe(&mut histbuf); + + catch_unwind(move || unwind_safe.write(Dropper::new(false))).unwrap_err(); + drop(histbuf); + assert_eq!(Dropper::count(), 0); + } + fn _test_variance<'a: 'b, 'b>(x: HistoryBuf<&'a (), 42>) -> HistoryBuf<&'b (), 42> { x } diff --git a/src/vec/mod.rs b/src/vec/mod.rs index 37d0ba820c..86c81ba835 100644 --- a/src/vec/mod.rs +++ b/src/vec/mod.rs @@ -745,7 +745,7 @@ impl + ?Sized> VecInner { unsafe { // Note: It's intentional that this is `>` and not `>=`. // Changing it to `>=` has negative performance - // implications in some cases. See rust-lang/rust#78884 for more. + // implications in some cases. See https://github.com/rust-lang/rust/issues/78884 for more. if len > self.len() { return; } @@ -1571,11 +1571,21 @@ where impl Drop for IntoIter { fn drop(&mut self) { + let len = self.vec.len; + self.vec.len = LenT::ZERO; + // SAFETY: `self.vec` is a vec where the first `self.next` elements have + // been moved through iteration. The rest a have not been consumed yet, + // so it is safe to create a mutable slice and drop them + // + // `self.vec.len` is set to `0` before dropping the elements so that + // a panicking `Drop` implementation does not result in inconsistent + // state during unwinding that could lead to undefined behaviour. unsafe { - // Drop all the elements that have not been moved out of vec - ptr::drop_in_place(&mut self.vec.as_mut_slice()[self.next.into_usize()..]); - // Prevent dropping of other elements - self.vec.len = LenT::ZERO; + let remaining = slice::from_raw_parts_mut( + self.vec.as_mut_ptr().add(self.next.into_usize()), + (len - self.next).into_usize(), + ); + ptr::drop_in_place(remaining); } } } @@ -1805,6 +1815,10 @@ where #[cfg(test)] mod tests { use core::fmt::Write; + use std::{ + panic::catch_unwind, + sync::atomic::{AtomicI32, Ordering::Relaxed}, + }; use static_assertions::assert_not_impl_any; @@ -1848,7 +1862,7 @@ mod tests { } #[test] - fn drop() { + fn test_drop() { droppable!(); { @@ -2385,6 +2399,41 @@ mod tests { } } + // Tests that a panic in a downstream drop implementation does not lead to an + // inconsistent state during unwinding that could lead to undefined behaviour. + // For more info see https://github.com/rust-embedded/heapless/issues/659 + #[test] + fn test_use_after_free_intoiter_drop() { + static COUNT: AtomicI32 = AtomicI32::new(0); + + #[derive(Debug)] + #[allow(unused)] + struct Dropper(); + + impl Dropper { + fn new() -> Self { + COUNT.fetch_add(1, Relaxed); + Self() + } + fn count() -> i32 { + COUNT.load(Relaxed) + } + } + impl Drop for Dropper { + fn drop(&mut self) { + COUNT.fetch_sub(1, Relaxed); + panic!("Testing panicking"); + } + } + + let mut vec = Vec::::new(); + vec.push(Dropper::new()).unwrap(); + let iterator = vec.into_iter(); + + catch_unwind(move || drop(iterator)).unwrap_err(); + assert_eq!(Dropper::count(), 0); + } + fn _test_variance<'a: 'b, 'b>(x: Vec<&'a (), 42>) -> Vec<&'b (), 42> { x }