From a78ed6b91eb1401f80c3d59331c022611b81b403 Mon Sep 17 00:00:00 2001 From: Haseeb Khalid Date: Tue, 12 May 2026 19:24:27 -0400 Subject: [PATCH 1/2] Fix UB in Iterator::map_windows clone on element clone panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Buffer::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`. --- library/core/src/iter/adapters/map_windows.rs | 15 +++- .../tests/iter/adapters/map_windows.rs | 81 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/library/core/src/iter/adapters/map_windows.rs b/library/core/src/iter/adapters/map_windows.rs index 097a0745c61c7..5aa2e866e6f0f 100644 --- a/library/core/src/iter/adapters/map_windows.rs +++ b/library/core/src/iter/adapters/map_windows.rs @@ -194,11 +194,24 @@ impl Buffer { impl Clone for Buffer { 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 } } diff --git a/library/coretests/tests/iter/adapters/map_windows.rs b/library/coretests/tests/iter/adapters/map_windows.rs index 994ec087e5e8b..61e58e31ab7d0 100644 --- a/library/coretests/tests/iter/adapters/map_windows.rs +++ b/library/coretests/tests/iter/adapters/map_windows.rs @@ -143,6 +143,87 @@ mod drop_checks { check::<5>(5, 1); check::<5>(5, 4); } + + /// Regression test for . + /// + /// `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(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] From cfa95bcc7a3b95d883b7b01b02bc21ad5de8d46c Mon Sep 17 00:00:00 2001 From: Haseeb Khalid Date: Tue, 12 May 2026 19:48:13 -0400 Subject: [PATCH 2/2] Address review feedback on map_windows clone panic-safety - 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`. --- library/core/src/iter/adapters/map_windows.rs | 21 +-- .../tests/iter/adapters/map_windows.rs | 155 ++++++++++-------- 2 files changed, 91 insertions(+), 85 deletions(-) diff --git a/library/core/src/iter/adapters/map_windows.rs b/library/core/src/iter/adapters/map_windows.rs index 5aa2e866e6f0f..958a410682694 100644 --- a/library/core/src/iter/adapters/map_windows.rs +++ b/library/core/src/iter/adapters/map_windows.rs @@ -194,24 +194,19 @@ impl Buffer { impl Clone for Buffer { 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(); + // 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(); - // 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`. + // 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(cloned); + buffer.as_uninit_array_mut().write(array); buffer } } diff --git a/library/coretests/tests/iter/adapters/map_windows.rs b/library/coretests/tests/iter/adapters/map_windows.rs index 61e58e31ab7d0..78922aad553a3 100644 --- a/library/coretests/tests/iter/adapters/map_windows.rs +++ b/library/coretests/tests/iter/adapters/map_windows.rs @@ -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(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); @@ -144,85 +203,37 @@ mod drop_checks { check::<5>(5, 4); } - /// Regression test for . - /// - /// `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. + /// Regression test for : + /// a panic inside `Clone::clone` during `Buffer::clone` must not drop + /// uninitialized slots and must not double-drop already-cloned elements. #[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) - } - } + 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); - 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); - } - } + check_clone_panic::<2>(2, 1, 1); + check_clone_panic::<2>(2, 1, 2); - #[track_caller] - fn run(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(); + check_clone_panic::<3>(3, 1, 1); + check_clone_panic::<3>(3, 1, 3); - // 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(); - })); - }); - } + 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); - // Panic at each position within the window, for several window sizes. - run::<1>(1); + // `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); - run::<2>(1); - run::<2>(2); + check_clone_panic::<3>(6, 4, 1); + check_clone_panic::<3>(6, 4, 3); - run::<4>(1); - run::<4>(2); - run::<4>(3); - run::<4>(4); + check_clone_panic::<4>(8, 5, 1); + check_clone_panic::<4>(8, 5, 4); } }