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

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

@@ -1,5 +1,5 @@
 use crate::iter::FusedIterator;
-use crate::mem::MaybeUninit;
+use crate::mem::{ManuallyDrop, MaybeUninit};
 use crate::{fmt, ptr};
 
 /// An iterator over the mapped windows of another iterator.
@@ -30,14 +30,19 @@ 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`.
+//
+// FIXME make these unsafe fields once that feature is ready
 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 +199,22 @@ 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.
+        // (We could instead just construct `self.as_array_ref().clone()`
+        // as a local on the stack before creating the buffer.
+        // That would avoid the leak amplification,
+        // but maybe also make it harder to optimize out the local/temporary?)
         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,47 @@ mod drop_checks {
         check::<5>(5, 1);
         check::<5>(5, 4);
     }
+
+    /// Regression test for #156501
+    #[test]
+    fn panicking_clone() {
+        static DROP_COUNTER: AtomicUsize = AtomicUsize::new(0);
+
+        struct PanickingClone(u8);
+
+        impl Clone for PanickingClone {
+            fn clone(&self) -> Self {
+                panic!(
+                    "⚞(· <:::> ·)⚟ aaaaaah its the turbofish monster!!! its gonna eat us all!!!1!"
+                );
+            }
+        }
+
+        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());
+
+        // current impl does leak amplification
+        assert_eq!(DROP_COUNTER.load(Relaxed), 0);
+    }
 }
 
 #[test]