Skip to content

Don't drop uninit memory when MapWindows::clone panics#156588

Open
Jules-Bertholet wants to merge 1 commit into
rust-lang:mainfrom
Jules-Bertholet:fix-panicky-mapwindows-clone
Open

Don't drop uninit memory when MapWindows::clone panics#156588
Jules-Bertholet wants to merge 1 commit into
rust-lang:mainfrom
Jules-Bertholet:fix-panicky-mapwindows-clone

Conversation

@Jules-Bertholet
Copy link
Copy Markdown
Contributor

Fixes #156501, using the approach suggested in @bjorn3's comment #156517 (comment)

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 14, 2026

The Miri subtree was changed

cc @rust-lang/miri

@rustbot 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 14, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 14, 2026

r? @scottmcm

rustbot has assigned @scottmcm.
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: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, scottmcm

Copy link
Copy Markdown
Member

@RalfJung RalfJung May 15, 2026

Choose a reason for hiding this comment

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

We typically add tests for std regressions in the std (or better, core/alloc) test suite. There are already some Miri-targeted tests there.

View changes since the review

Comment on lines +33 to +34
/// `Buffer` uses two times of space to reduce moves among the iterations.
/// `Buffer<T, N>` is semantically `[MaybeUninit<T>; 2 * N]`. However, due
Copy link
Copy Markdown
Member

@RalfJung RalfJung May 15, 2026

Choose a reason for hiding this comment

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

The second sentence mostly repeats the first one, doesn't it? (and also the first one isn't quite grammatical)

Suggested change
/// `Buffer` uses two times of space to reduce moves among the iterations.
/// `Buffer<T, N>` is semantically `[MaybeUninit<T>; 2 * N]`. However, due
/// `Buffer<T, N>` is semantically `[MaybeUninit<T>; 2 * N]`. This helps
/// reduce moves while iterating. However, due

View changes since the review

Comment on lines +43 to +44
//
// FIXME make these unsafe fields once that feature is ready
Copy link
Copy Markdown
Member

@RalfJung RalfJung May 15, 2026

Choose a reason for hiding this comment

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

Suggested change
//
// FIXME make these unsafe fields once that feature is ready

I don't think it's worth adding this FIXME to very single type with an invariant.

View changes since the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants