Skip to content

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

Closed
Paladynee wants to merge 2 commits into
rust-lang:mainfrom
Paladynee:fix/map_windows-soundness
Closed

Refactor underlying data of map_windows::Buffer into RawBuffer#156533
Paladynee wants to merge 2 commits into
rust-lang:mainfrom
Paladynee:fix/map_windows-soundness

Conversation

@Paladynee
Copy link
Copy Markdown
Contributor

@Paladynee Paladynee commented May 13, 2026

Refactor the liveness and access invariants of Buffer into RawBuffer. This lets us initialize a Buffer without dropping any uninitialized data if anything panics transiently.

Fixes #156501.

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

rustbot commented May 13, 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, nia-e, scottmcm

@rustbot

This comment has been minimized.

@Paladynee
Copy link
Copy Markdown
Contributor Author

r? @nia-e

@rustbot rustbot assigned nia-e and unassigned scottmcm May 13, 2026
@rust-log-analyzer

This comment has been minimized.

Refactor the liveness and access invariants of Buffer into RawBuffer.
This lets us initialize a Buffer without dropping it if it panics transiently.

Fixes rust-lang#156501.
@Paladynee Paladynee force-pushed the fix/map_windows-soundness branch from bb69c9d to 4fad2ca Compare May 13, 2026 09:40
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 13, 2026

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 13, 2026

I think this would make optimizing away the copy just as hard for the optimizer as with #156517.

@Paladynee
Copy link
Copy Markdown
Contributor Author

Paladynee commented May 13, 2026

I think this would make optimizing away the copy just as hard for the optimizer as with #156517.

do you mean Self::new, the as_array_ref extraction, or some other copy that i'm not aware of?

is ManuallyDrop special cased in the compiler to have better copy elision?

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 13, 2026

With this PR there is first a copy during the clone. Then a copy into new_raw, a copy for Self::new and finally a copy into the return area. The copy during clone is unavoidable. The copy into new_raw can probably be folded into the clone copy and the copy into the return area can probably be folded into creating the output of Self::new. My gut feeling says that the copy for Self::new probably won't be optimized out though.

For the other PR there is first a copy during the clone. Then a copy into buffer and finally a copy to the return area. Again the copy to the return area will probably be optimized away. The copy into buffer might not be.

is ManuallyDrop special cased in the compiler to have better copy elision?

It isn't. The ManuallyDrop would just be to suppress the Drop impl that is causing the unsoundness.

To be clear I haven't checked if my suggestion actually helps.

@Paladynee
Copy link
Copy Markdown
Contributor Author

Paladynee commented May 13, 2026

My gut feeling says that the copy for Self::new probably won't be optimized out though.

I can replace the calls to new with struct literals, though it would need invariant clarification. is there a guideline for documenting struct literals with invariants (like // SAFETY comments)?

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 13, 2026

Self::new will probably be inlined, so manually inlining shouldn't matter.

@nia-e
Copy link
Copy Markdown
Member

nia-e commented May 13, 2026

I'm not convinced that this level of overhaul is really necessary for this. The bug can probably be addressed much more simply by implementing bjorn3's suggestion from the other PR; this is a lot of churn in a very delicate part of the codebase. I'll take another look at an overhaul like this if a simple fix isn't possible, but I'd like to see the reasoning there first.

@nia-e nia-e closed this May 13, 2026
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2026
@Paladynee Paladynee deleted the fix/map_windows-soundness branch May 14, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

6 participants