Fix unwind vulnerability

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`

src/history_buf.rs

@@ -385,9 +385,12 @@ impl<T, S: HistoryBufStorage<T> + ?Sized> HistoryBufInner<T, S> {
 
     /// 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::<Dropper, 5>::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
     }

src/vec/mod.rs

@@ -745,7 +745,7 @@ impl<T, LenT: LenType, S: VecStorage<T> + ?Sized> VecInner<T, LenT, S> {
         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<T, LenT: LenType, const N: usize> Drop for IntoIter<T, N, LenT> {
     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::<Dropper, 5>::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
     }