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` 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.
+///
+/// # 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)
     }
 }
 

src/tools/miri/tests/pass/issues/issue-156501-mapwindows-panicking-clone.rs

@@ -0,0 +1,40 @@
+#![feature(iter_map_windows)]
+
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+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, Ordering::Relaxed);
+    }
+}
+
+fn main() {
+    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(Ordering::Relaxed), 0);
+}

src/tools/miri/tests/pass/issues/issue-156501-mapwindows-panicking-clone.stderr

@@ -0,0 +1,5 @@
+
+thread 'main' ($TID) panicked at tests/pass/issues/issue-156501-mapwindows-panicking-clone.rs:LL:CC:
+⚞(· <:::> ·)⚟ aaaaaah its the turbofish monster!!! its gonna eat us all!!!1!
+note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect