diff --git a/library/core/src/iter/adapters/map_windows.rs b/library/core/src/iter/adapters/map_windows.rs index 097a0745c61c7..eabdb21ad7c3a 100644 --- a/library/core/src/iter/adapters/map_windows.rs +++ b/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 { buffer: Option>, } -// `Buffer` uses two times of space to reduce moves among the iterations. -// `Buffer` is semantically `[MaybeUninit; 2 * N]`. However, due -// to limitations of const generics, we use this different type. Note that -// it has the same underlying memory layout. +/// `Buffer` is semantically `[MaybeUninit; 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 { - // 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; N]; 2], start: usize, } @@ -194,12 +199,22 @@ impl Buffer { impl Clone for Buffer { 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) } } diff --git a/library/coretests/tests/iter/adapters/map_windows.rs b/library/coretests/tests/iter/adapters/map_windows.rs index 994ec087e5e8b..431beb5efd608 100644 --- a/library/coretests/tests/iter/adapters/map_windows.rs +++ b/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]