Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
10 changes: 9 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,19 @@ 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 first: if `T::clone`
// panics partway through, no `Buffer` has been constructed yet, so
// `Buffer::drop` cannot run over uninitialized slots.
let array = self.as_array_ref().clone();

// From here through `MaybeUninit::write` below, `buffer` exists with
// its `Drop` armed over `[start..start + N]` uninitialized slots.
// This is sound only because no panic-capable code runs in between.
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(array);
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
92 changes: 92 additions & 0 deletions library/coretests/tests/iter/adapters/map_windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,65 @@ mod drop_checks {
info.check();
}

/// `DropCheck`-style element whose `Clone::clone` panics on the
/// `panic_on`-th call (1-indexed). Used to exercise panic-safety of
/// adapters that internally clone an active window of `T`s.
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!("intended panic in 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);
}
}

/// Build a `map_windows::<_, _, N>` over `source_len` `ClonePanicker`s,
/// advance the iterator `advance` times (so the internal `Buffer`'s
/// `start` reaches the desired offset), then assert that cloning the
/// iterator panics on the `panic_on`-th `Clone::clone` call.
/// `check_drops` then verifies no double-drop and no leak.
#[track_caller]
fn check_clone_panic<const N: usize>(source_len: usize, advance: usize, panic_on: usize) {
check_drops(|info| {
let clone_count = AtomicUsize::new(0);
let mut iter = (0..source_len)
.map(|_| ClonePanicker::new(info, &clone_count, panic_on))
.map_windows(|_: &[_; N]| ());
for _ in 0..advance {
iter.next();
}
let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
let _clone = iter.clone();
}));
assert!(r.is_err(), "clone should have panicked at element {panic_on}");
});
}

#[test]
fn no_iter_panic_n1() {
check::<1>(0, 100);
Expand Down Expand Up @@ -143,6 +202,39 @@ mod drop_checks {
check::<5>(5, 1);
check::<5>(5, 4);
}

/// Regression test for <https://github.com/rust-lang/rust/issues/156501>:
/// a panic inside `Clone::clone` during `Buffer::clone` must not drop
/// uninitialized slots and must not double-drop already-cloned elements.
#[test]
fn panic_in_clone() {
// `start == 0` (initial buffer state after the first `next()`).
// Panic at every position within the window for several window sizes.
check_clone_panic::<1>(1, 1, 1);

check_clone_panic::<2>(2, 1, 1);
check_clone_panic::<2>(2, 1, 2);

check_clone_panic::<3>(3, 1, 1);
check_clone_panic::<3>(3, 1, 3);

check_clone_panic::<4>(4, 1, 1);
check_clone_panic::<4>(4, 1, 2);
check_clone_panic::<4>(4, 1, 3);
check_clone_panic::<4>(4, 1, 4);

// `start == N` (buffer half-wrapped: `as_uninit_array_mut()` writes
// into the second half of the backing storage). This is a
// physically distinct destination region from the `start == 0` case.
check_clone_panic::<2>(4, 3, 1);
check_clone_panic::<2>(4, 3, 2);

check_clone_panic::<3>(6, 4, 1);
check_clone_panic::<3>(6, 4, 3);

check_clone_panic::<4>(8, 5, 1);
check_clone_panic::<4>(8, 5, 4);
}
}

#[test]
Expand Down
Loading