Rollup merge of rust-lang#156588 - Jules-Bertholet:fix-panicky-mapwindows-clone, r=Mark-Simulacrum

Don't drop uninit memory when `MapWindows::clone` panics

Fixes rust-lang#156501, using the approach suggested in @bjorn3's comment rust-lang#156517 (comment)

Author: JonathanBrouwer

library/core/src/iter/adapters/map_windows.rs

@@ -1,5 +1,5 @@
 use crate::iter::FusedIterator;
-use crate::mem::{MaybeUninit, SizedTypeProperties};
+use crate::mem::{ManuallyDrop, MaybeUninit, SizedTypeProperties};
 use crate::{fmt, ptr};
 
 /// An iterator over the mapped windows of another iterator.
@@ -30,14 +30,17 @@ struct MapWindowsInner<I: Iterator, const N: usize> {
     buffer: Option<Buffer<I::Item, N>>,
 }
 
-// `Buffer` uses two times of space to reduce moves among the iterations.
-// `Buffer<T, N>` is semantically `[MaybeUninit<T>; 2 * N]`. However, due
-// to limitations of const generics, we use this different type. Note that
-// it has the same underlying memory layout.
+/// `Buffer<T, N>` is semantically `[MaybeUninit<T>; 2 * N]`. This helps
+/// reduce moves while iterating. However, due
+/// to limitations of const generics, we use this different type. Note that
+/// it has the same underlying memory layout.
+///
+/// # Safety invariant
+///
+/// `self.buffer[self.start..self.start + N]` must be initialized,
+/// with all other elements being uninitialized. This also
+/// implies that `self.start <= N`.
 struct Buffer<T, const N: usize> {
-    // Invariant: `self.buffer[self.start..self.start + N]` is initialized,
-    // with all other elements being uninitialized. This also
-    // implies that `self.start <= N`.
     buffer: [[MaybeUninit<T>; N]; 2],
     start: usize,
 }
@@ -194,12 +197,18 @@ impl<T, const N: usize> Buffer<T, N> {
 
 impl<T: Clone, const N: usize> Clone for Buffer<T, N> {
     fn clone(&self) -> Self {
-        let mut buffer = Buffer {
+        // Use `ManuallyDrop` until buffer is fully written to avoid dropping uninitialized elements on panic.
+        // (See `Buffer` rustdoc for safety invariant)
+        let mut buffer = ManuallyDrop::new(Buffer {
             buffer: [[const { MaybeUninit::uninit() }; N], [const { MaybeUninit::uninit() }; N]],
             start: self.start,
-        };
+        });
+
+        // `clone()` could panic; `ManuallyDrop` guards against that.
         buffer.as_uninit_array_mut().write(self.as_array_ref().clone());
-        buffer
+
+        // We initialized the buffer above, so we are good now
+        ManuallyDrop::into_inner(buffer)
     }
 }
 

library/coretests/tests/iter/adapters/map_windows.rs

@@ -5,7 +5,7 @@ use std::sync::atomic::Ordering::SeqCst;
 mod drop_checks {
     //! These tests mainly make sure the elements are correctly dropped.
 
-    use std::sync::atomic::Ordering::SeqCst;
+    use std::sync::atomic::Ordering::{Relaxed, SeqCst};
     use std::sync::atomic::{AtomicBool, AtomicUsize};
 
     #[derive(Debug)]
@@ -143,6 +143,51 @@ mod drop_checks {
         check::<5>(5, 1);
         check::<5>(5, 4);
     }
+
+    /// Regression test for #156501
+    #[test]
+    fn panicking_clone() {
+        static CLONE_COUNTER: AtomicUsize = AtomicUsize::new(0);
+        static DROP_COUNTER: AtomicUsize = AtomicUsize::new(0);
+
+        struct PanickingClone(u8);
+
+        impl Clone for PanickingClone {
+            fn clone(&self) -> Self {
+                if CLONE_COUNTER.fetch_add(1, Relaxed) == 3 {
+                    panic!(
+                        "⚞(· <:::> ·)⚟ aaaaaah its the turbofish monster!!! its gonna eat us all!!!1!"
+                    );
+                }
+
+                Self(self.0)
+            }
+        }
+
+        impl Drop for PanickingClone {
+            fn drop(&mut self) {
+                assert_eq!(self.0, 42);
+
+                DROP_COUNTER.fetch_add(1, Relaxed);
+            }
+        }
+
+        let array = [const { PanickingClone(42) }; 17];
+        let mut iter = array.into_iter().map_windows::<_, (), 17>(|_| ());
+
+        // initialize the buffer
+        iter.next();
+
+        let result = std::panic::catch_unwind(|| {
+            // now do the clones and panic.
+            // this should't try to drop uninitialized memory
+            let _ = iter.clone();
+        });
+
+        assert!(result.is_err());
+
+        assert_eq!(DROP_COUNTER.load(Relaxed), 3);
+    }
 }
 
 #[test]