Merge pull request rust-embedded#647 from sgued/646-clear-use-after-free

Fix possible use after free in `clear` functions

Author: sgued

CHANGELOG.md

@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
+
+- 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/deque.rs

@@ -243,11 +243,18 @@ impl<T, S: VecStorage<T> + ?Sized> DequeInner<T, S> {
 
     /// Clears the deque, removing all values.
     pub fn clear(&mut self) {
-        // safety: we're immediately setting a consistent empty state.
-        unsafe { self.drop_contents() }
-        self.front = 0;
-        self.back = 0;
-        self.full = false;
+        struct Guard<'a, T, S: VecStorage<T> + ?Sized>(&'a mut DequeInner<T, S>);
+        impl<'a, T, S: VecStorage<T> + ?Sized> Drop for Guard<'a, T, S> {
+            fn drop(&mut self) {
+                self.0.front = 0;
+                self.0.back = 0;
+                self.0.full = false;
+            }
+        }
+        let this = Guard(self);
+        // SAFETY: Guard will be dropped and lead to a consistent empty state even in the case of a
+        // panic leading to unwinding during the dropping of an item
+        unsafe { this.0.drop_contents() }
     }
 
     /// Drop all items in the `Deque`, leaving the state `back/front/full` unmodified.
@@ -1306,6 +1313,12 @@ impl<T, const NS: usize, const ND: usize> TryFrom<[T; NS]> for Deque<T, ND> {
 
 #[cfg(test)]
 mod tests {
+    use std::{
+        mem::ManuallyDrop,
+        panic::{catch_unwind, AssertUnwindSafe},
+        sync::atomic::{AtomicI32, Ordering::Relaxed},
+    };
+
     use super::Deque;
     use crate::CapacityError;
     use static_assertions::assert_not_impl_any;
@@ -1324,7 +1337,7 @@ mod tests {
     }
 
     #[test]
-    fn drop() {
+    fn test_drop() {
         droppable!();
 
         {
@@ -1629,20 +1642,23 @@ mod tests {
 
     #[test]
     fn clear() {
-        let mut q: Deque<i32, 4> = Deque::new();
+        droppable!();
+        let mut q: Deque<Droppable, 4> = Deque::new();
         assert_eq!(q.len(), 0);
 
-        q.push_back(0).unwrap();
-        q.push_back(1).unwrap();
-        q.push_back(2).unwrap();
-        q.push_back(3).unwrap();
+        q.push_back(Droppable::new()).unwrap();
+        q.push_back(Droppable::new()).unwrap();
+        q.push_back(Droppable::new()).unwrap();
+        q.push_back(Droppable::new()).unwrap();
         assert_eq!(q.len(), 4);
 
         q.clear();
         assert_eq!(q.len(), 0);
+        assert_eq!(Droppable::count(), 0);
 
-        q.push_back(0).unwrap();
+        q.push_back(Droppable::new()).unwrap();
         assert_eq!(q.len(), 1);
+        assert_eq!(Droppable::count(), 1);
     }
 
     #[test]
@@ -2275,4 +2291,42 @@ mod tests {
         assert_eq!(deq.pop_back_if(|_| true), None);
         assert!(deq.is_empty());
     }
+
+    #[test]
+    fn test_use_after_free_clear() {
+        // See https://github.com/rust-embedded/heapless/issues/646
+
+        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 deque = Deque::<Dropper, 5>::new();
+        deque.push_back(Dropper::new()).unwrap();
+        // Don't panic on dropping of the deque
+        let mut deque = ManuallyDrop::new(deque);
+        let mut unwind_safe_deque = AssertUnwindSafe(&mut deque);
+
+        catch_unwind(move || unwind_safe_deque.clear()).unwrap_err();
+        assert_eq!(deque.len(), 0);
+        deque.clear();
+        assert_eq!(Dropper::count(), 0);
+    }
 }

src/history_buf.rs

@@ -308,24 +308,30 @@ impl<T: Copy, S: HistoryBufStorage<T> + ?Sized> HistoryBufInner<T, S> {
 
     /// Clears the buffer, replacing every element with the given value.
     pub fn clear_with(&mut self, t: T) {
-        // SAFETY: we reset the values just after
-        unsafe { self.drop_contents() };
-        self.write_at = 0;
-        self.filled = true;
+        self.clear();
 
         for d in self.data.borrow_mut() {
             *d = MaybeUninit::new(t);
         }
+        self.write_at = 0;
+        self.filled = true;
     }
 }
 
 impl<T, S: HistoryBufStorage<T> + ?Sized> HistoryBufInner<T, S> {
     /// Clears the buffer
     pub fn clear(&mut self) {
-        // SAFETY: we reset the values just after
-        unsafe { self.drop_contents() };
-        self.write_at = 0;
-        self.filled = false;
+        struct Guard<'a, T, S: HistoryBufStorage<T> + ?Sized>(&'a mut HistoryBufInner<T, S>);
+        impl<'a, T, S: HistoryBufStorage<T> + ?Sized> Drop for Guard<'a, T, S> {
+            fn drop(&mut self) {
+                self.0.write_at = 0;
+                self.0.filled = false;
+            }
+        }
+        let this = Guard(self);
+        // SAFETY: Guard will be dropped and lead to a consistent empty state even in the case of a
+        // panic leading to unwinding during the dropping of an item
+        unsafe { this.0.drop_contents() };
     }
 }
 
@@ -659,6 +665,11 @@ mod tests {
         fmt::Debug,
         sync::atomic::{AtomicUsize, Ordering},
     };
+    use std::{
+        mem::ManuallyDrop,
+        panic::{catch_unwind, AssertUnwindSafe},
+        sync::atomic::AtomicI32,
+    };
 
     use static_assertions::assert_not_impl_any;
 
@@ -983,6 +994,44 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_use_after_free_clear() {
+        // See https://github.com/rust-embedded/heapless/issues/646
+
+        static COUNT: AtomicI32 = AtomicI32::new(0);
+
+        #[derive(Debug)]
+        #[allow(unused)]
+        struct Dropper();
+
+        impl Dropper {
+            fn new() -> Self {
+                COUNT.fetch_add(1, Ordering::Relaxed);
+                Self()
+            }
+            fn count() -> i32 {
+                COUNT.load(Ordering::Relaxed)
+            }
+        }
+        impl Drop for Dropper {
+            fn drop(&mut self) {
+                COUNT.fetch_sub(1, Ordering::Relaxed);
+                panic!("Testing  panicking");
+            }
+        }
+
+        let mut history_buf = HistoryBuf::<Dropper, 5>::new();
+        history_buf.write(Dropper::new());
+        // Don't panic on dropping of the history_buf
+        let mut history_buf = ManuallyDrop::new(history_buf);
+        let mut unwind_safe_history_buf = AssertUnwindSafe(&mut history_buf);
+
+        catch_unwind(move || unwind_safe_history_buf.clear()).unwrap_err();
+        assert_eq!(history_buf.len(), 0);
+        history_buf.clear();
+        assert_eq!(Dropper::count(), 0);
+    }
+
     fn _test_variance<'a: 'b, 'b>(x: HistoryBuf<&'a (), 42>) -> HistoryBuf<&'b (), 42> {
         x
     }

src/index_map.rs

@@ -978,10 +978,18 @@ impl<K, V, S, const N: usize> IndexMap<K, V, S, N> {
     /// assert!(a.is_empty());
     /// ```
     pub fn clear(&mut self) {
-        self.core.entries.clear();
-        for pos in self.core.indices.iter_mut() {
-            *pos = None;
+        struct Guard<'a, K, V, S, const N: usize>(&'a mut IndexMap<K, V, S, N>);
+        impl<'a, K, V, S, const N: usize> Drop for Guard<'a, K, V, S, N> {
+            fn drop(&mut self) {
+                for pos in self.0.core.indices.iter_mut() {
+                    *pos = None;
+                }
+            }
         }
+        let this = Guard(self);
+        // SAFETY: Guard will be dropped and lead to a consistent empty state even in the case of a
+        // panic leading to unwinding during the dropping of an item
+        this.0.core.entries.clear();
     }
 }
 
@@ -1617,6 +1625,11 @@ where
 #[cfg(test)]
 mod tests {
     use core::mem;
+    use std::{
+        mem::ManuallyDrop,
+        panic::{catch_unwind, AssertUnwindSafe},
+        sync::atomic::{AtomicI32, Ordering},
+    };
 
     use static_assertions::assert_not_impl_any;
 
@@ -2058,4 +2071,42 @@ mod tests {
         assert_eq!(map.len(), 1);
         assert_eq!(map.get(&1), Some(&10));
     }
+
+    #[test]
+    fn test_use_after_free_clear() {
+        // See https://github.com/rust-embedded/heapless/issues/646
+
+        static COUNT: AtomicI32 = AtomicI32::new(0);
+
+        #[derive(Debug, Hash, PartialEq, Eq)]
+        #[allow(unused)]
+        struct Dropper();
+
+        impl Dropper {
+            fn new() -> Self {
+                COUNT.fetch_add(1, Ordering::Relaxed);
+                Self()
+            }
+            fn count() -> i32 {
+                COUNT.load(Ordering::Relaxed)
+            }
+        }
+        impl Drop for Dropper {
+            fn drop(&mut self) {
+                COUNT.fetch_sub(1, Ordering::Relaxed);
+                panic!("Testing  panicking");
+            }
+        }
+
+        let mut history_buf = FnvIndexMap::<Dropper, u32, 8>::new();
+        history_buf.insert(Dropper::new(), 0).unwrap();
+        // Don't panic on dropping of the history_buf
+        let mut history_buf = ManuallyDrop::new(history_buf);
+        let mut unwind_safe_history_buf = AssertUnwindSafe(&mut history_buf);
+
+        catch_unwind(move || unwind_safe_history_buf.clear()).unwrap_err();
+        assert_eq!(history_buf.len(), 0);
+        history_buf.clear();
+        assert_eq!(Dropper::count(), 0);
+    }
 }

src/test_helpers.rs

@@ -2,7 +2,7 @@ macro_rules! droppable {
     () => {
         static COUNT: core::sync::atomic::AtomicI32 = core::sync::atomic::AtomicI32::new(0);
 
-        #[derive(Eq, Ord, PartialEq, PartialOrd)]
+        #[derive(Eq, Ord, PartialEq, PartialOrd, Debug, Hash)]
         struct Droppable(i32);
         impl Droppable {
             fn new() -> Self {