Fix UB in Iterator::map_windows clone on element clone panic by HaseebKhalid1507 · Pull Request #156517 · rust-lang/rust

HaseebKhalid1507 · 2026-05-12T23:27:18Z

Buffer<T, N>::clone in library/core/src/iter/adapters/map_windows.rs
constructs a Buffer with start = self.start (which arms the Drop
impl over start..start + N) before calling <[T; N] as Clone>::clone
to populate that range. If the array-clone panics, unwinding drops the
half-built Buffer and Buffer::drop calls ptr::drop_in_place on
uninitialized memory — UB reachable from safe Rust under
#![feature(iter_map_windows)].

The fix clones the window into a stack-local [T; N] first. The array
Clone impl is itself panic-safe (it drops already-cloned predecessors
on failure), so a mid-clone panic unwinds before any Buffer exists.
Only after the array clone succeeds is the destination Buffer
constructed and the array moved into its MaybeUninit storage via
MaybeUninit::write, which is infallible.

The regression test lives in the existing
coretests::iter::adapters::map_windows::drop_checks module and reuses
its DropInfo harness. It uses a ClonePanicker element that panics
on its k-th Clone::clone call, parameterized over both the window
size N and the panic position within the window. Without the fix,
this test fails under Miri with the same uninitialized-read diagnostic
as the issue's PoC.

Validated locally:

repro from the issue → segfault on native nightly, Miri reports UB
standalone extraction of Buffer + fixed clone → Miri clean
new regression test against the unfixed core → Miri UB
./x test tidy → passes
rustfmt --edition 2024 --config-path rustfmt.toml → no diff

@rustbot label +I-unsound +T-libs

`Buffer<T, N>::clone` constructed the destination `Buffer` (with `Drop` armed via `start = self.start`) *before* cloning the active window into its `MaybeUninit` storage. If `<[T; N] as Clone>::clone` panicked partway through, unwinding dropped the destination `Buffer`, whose `Drop` impl then called `ptr::drop_in_place` over `[start..start + N]` uninitialized slots — UB reachable from safe `#![feature(iter_map_windows)]` code. Clone the window into a stack-local `[T; N]` first. `<[T; N] as Clone>` is itself panic-safe: a clone failure on the k-th element drops the k-1 already-cloned predecessors. Only after the array clone succeeds do we construct the destination `Buffer` and move the array into its storage. `MaybeUninit::write` cannot panic, so the `Buffer::drop` invariant ("`[start..start + N]` initialized") holds for the entire lifetime of the new `Buffer`.

rustbot · 2026-05-12T23:27:23Z

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: libs
libs expanded to 7 candidates
Random selection from Mark-Simulacrum, nia-e

Copilot

Pull request overview

This PR fixes a soundness issue in the unstable Iterator::map_windows implementation where Buffer<T, N>::clone could drop uninitialized memory if an element’s Clone::clone panicked during cloning, making UB reachable from safe Rust. The fix makes Buffer::clone panic-safe by cloning into a temporary [T; N] first and only constructing/writing into the destination Buffer after cloning succeeds, and adds a regression test to cover the panic-unwind path.

Changes:

Make Buffer<T, N>::clone panic-safe by cloning the active window into a temporary [T; N] before constructing the destination Buffer.
Add a regression test that forces panics at each position within the cloned window to ensure no uninitialized slots are dropped during unwinding.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
library/core/src/iter/adapters/map_windows.rs	Reworks `Buffer::clone` to avoid creating a drop-armed `Buffer` before the potentially-panicking array clone completes.
library/coretests/tests/iter/adapters/map_windows.rs	Adds a regression test that triggers panics during `iter.clone()` to detect uninitialized-drop UB and double-drop/leak issues.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

View changes since this review

+                let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+                    let _clone = iter.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,
        };


- Hoist `ClonePanicker` and `check_clone_panic` helpers to module scope inside `drop_checks`, matching the `DropCheck` / `check` / `check_drops` pattern of the surrounding tests. - Rename the regression test to `panic_in_clone`, parallel to the existing `panic_in_first_batch` / `panic_in_middle` naming. - Assert that `catch_unwind` returns `Err`. Without this, a future regression that silently stops panicking in `Buffer::clone` would let the test pass without exercising the panic path. - Add `start == N` test coverage. The buggy path was triggerable from two physically distinct destination regions inside `Buffer`'s backing storage depending on `self.start`; the previous test only covered `start == 0`. - Use the shorter `"intended panic in clone"` message, matching the existing `"intended panic"` convention in this file. - Tighten the comments on `Buffer::clone` and clarify that the `Buffer::drop` invariant holds only *after* the `MaybeUninit::write` call, not over the construction-to-write window. - Rename the `cloned` local to `array`.

asder8215 · 2026-05-13T00:03:52Z

Uncertain if this is an agent pushing commits to this PR or a human actually pushing the commits, but if someone is pushing these commits to this PR, you may want to read up the proposed guidelines for AI usage.

HaseebKhalid1507 · 2026-05-13T00:50:19Z

Hi, human here, not a bot. Sorry for the missing disclosure, this PR was AI assisted. The fix and tests were written by AI, but I reproduced the bug before patching, and I believe I understand what the code is doing.

bjorn3 · 2026-05-13T09:09:12Z

        };
-        buffer.as_uninit_array_mut().write(self.as_array_ref().clone());
+        buffer.as_uninit_array_mut().write(array);
        buffer


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

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

Copilot AI review requested due to automatic review settings May 12, 2026 23:27

rustbot assigned nia-e May 12, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 12, 2026

Copilot started reviewing on behalf of HaseebKhalid1507 May 12, 2026 23:28 View session

HaseebKhalid1507 mentioned this pull request May 12, 2026

MapWindows::clone is not panic-safe; panicking T::clone causes uninitialized memory to be dropped #156501

Open

Copilot AI reviewed May 12, 2026

View reviewed changes

bjorn3 reviewed May 13, 2026

View reviewed changes

bjorn3 mentioned this pull request May 13, 2026

Refactor underlying data of map_windows::Buffer into RawBuffer #156533

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix UB in Iterator::map_windows clone on element clone panic#156517

Fix UB in Iterator::map_windows clone on element clone panic#156517
HaseebKhalid1507 wants to merge 2 commits into
rust-lang:mainfrom
HaseebKhalid1507:fix-156501-map-windows-clone-uaf

HaseebKhalid1507 commented May 12, 2026 •

edited by rustbot

Loading

Uh oh!

rustbot commented May 12, 2026

Uh oh!

Copilot AI left a comment •

edited by rustbot

Loading

Uh oh!

asder8215 commented May 13, 2026

Uh oh!

HaseebKhalid1507 commented May 13, 2026

Uh oh!

bjorn3 May 13, 2026 •

edited by rustbot

Loading

Uh oh!

Paladynee May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

HaseebKhalid1507 commented May 12, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 12, 2026

Uh oh!

Copilot AI left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

asder8215 commented May 13, 2026

Uh oh!

HaseebKhalid1507 commented May 13, 2026

Uh oh!

bjorn3 May 13, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paladynee May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HaseebKhalid1507 commented May 12, 2026 •

edited by rustbot

Loading

Copilot AI left a comment •

edited by rustbot

Loading

bjorn3 May 13, 2026 •

edited by rustbot

Loading