Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion library/core/src/iter/adapters/map_windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,24 @@ impl<T, const N: usize> Buffer<T, N> {

impl<T: Clone, const N: usize> Clone for Buffer<T, N> {
fn clone(&self) -> Self {
// Clone the active window into a stack-local `[T; N]` *before*
// constructing the destination `Buffer`. `<[T; N] as Clone>::clone`
// is itself panic-safe: if cloning the k-th element panics, it drops
// the k-1 already-cloned elements in the temporary and propagates
// the unwind. At that point no `Buffer` has been built, so our
// `Drop` impl never runs over uninitialized slots.
let cloned = self.as_array_ref().clone();

// Only after the clone succeeds do we construct the destination
// `Buffer` and move the array into its `MaybeUninit` storage.
// `MaybeUninit::write` cannot panic, so the invariant
// `self.buffer[self.start..self.start + N]` is initialized holds
// for the entire lifetime of `buffer`.
let mut buffer = Buffer {
buffer: [[const { MaybeUninit::uninit() }; N], [const { MaybeUninit::uninit() }; N]],
start: self.start,
};
buffer.as_uninit_array_mut().write(self.as_array_ref().clone());
buffer.as_uninit_array_mut().write(cloned);
buffer
Copy link
Copy Markdown
Member

@bjorn3 bjorn3 May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to wrap Buffer in a ManuallyDrop and extract it out of the ManuallyDrop once the cloned values have been written. This would reduce stack memory usage, though it would leak memory on panic.

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont like this style of initializing an invalid Buffer, so i made another go at implementing a fix for this by abstracting the storage: #156533

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @bjorn3's suggestion is correct here, no need to reinvent the wheel. It's also the usual approach we use for this

}
}
Expand Down
81 changes: 81 additions & 0 deletions library/coretests/tests/iter/adapters/map_windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,87 @@ mod drop_checks {
check::<5>(5, 1);
check::<5>(5, 4);
}

/// Regression test for <https://github.com/rust-lang/rust/issues/156501>.
///
/// `Buffer::clone` used to construct the destination `Buffer` with
/// `start = self.start` *before* writing into its `MaybeUninit` storage.
/// If `<[T; N] as Clone>::clone` panicked partway through, unwinding
/// would drop the partially-constructed `Buffer`, and `Buffer::drop`
/// would treat `start..start + N` as initialized and call
/// `ptr::drop_in_place` on uninitialized memory.
#[test]
fn no_drop_of_uninit_on_clone_panic() {
/// A `DropCheck`-style element that panics on the `panic_on`-th
/// invocation of `Clone::clone` (1-indexed). Successful clones
/// register a new live element on `info` so `check_drops` catches
/// any double-drop or leak.
struct ClonePanicker<'a> {
info: &'a DropInfo,
clone_count: &'a AtomicUsize,
panic_on: usize,
was_dropped: bool,
}

impl<'a> ClonePanicker<'a> {
fn new(info: &'a DropInfo, clone_count: &'a AtomicUsize, panic_on: usize) -> Self {
info.alive_count.fetch_add(1, SeqCst);
Self { info, clone_count, panic_on, was_dropped: false }
}
}

impl<'a> Clone for ClonePanicker<'a> {
fn clone(&self) -> Self {
let n = self.clone_count.fetch_add(1, SeqCst) + 1;
if n == self.panic_on {
panic!("intentional panic from ClonePanicker::clone");
}
Self::new(self.info, self.clone_count, self.panic_on)
}
}

impl Drop for ClonePanicker<'_> {
fn drop(&mut self) {
if self.was_dropped {
self.info.dropped_twice.store(true, SeqCst);
}
self.was_dropped = true;
self.info.alive_count.fetch_sub(1, SeqCst);
}
}

#[track_caller]
fn run<const N: usize>(panic_on: usize) {
check_drops(|info| {
let clone_count = AtomicUsize::new(0);
let mut iter = (0..N)
.map(|_| ClonePanicker::new(info, &clone_count, panic_on))
.map_windows(|_: &[_; N]| ());

// Advance once so `inner.buffer` is `Some(Buffer<_, N>)`.
iter.next();

// Cloning panics partway through the window clone. The
// partially-constructed internal `Buffer` must not be
// dropped over uninitialized slots, and no element of
// `iter` may be dropped twice.
let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
let _clone = iter.clone();
}));
});
}

// Panic at each position within the window, for several window sizes.
run::<1>(1);

run::<2>(1);
run::<2>(2);

run::<4>(1);
run::<4>(2);
run::<4>(3);
run::<4>(4);
}
}

#[test]
Expand Down
Loading