Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero the REPARSE_MOUNTPOINT_DATA_BUFFER header #107900

Merged
merged 1 commit into from
Feb 11, 2023

Conversation

ChrisDenton
Copy link
Member

Makes sure the full header is correctly initialized.

Fixes #107884

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@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 Feb 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Makes sure the full header is correctly initialized, including reserve parameters.
@@ -1393,6 +1393,8 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> {
let mut data = Align8([MaybeUninit::<u8>::uninit(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might MU::zeroed be clearer here?

Suggested change
let mut data = Align8([MaybeUninit::<u8>::uninit(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
let mut data = Align8([MaybeUninit::<u8>::zeroed(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that would zero the entire buffer tho. Which I assume we're trying to avoid because it's large. Otherwise [0_u8, c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE] would probably be ok because it's a plain old data struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't matter too much if it's only used in tests but good point.

Copy link
Member

Choose a reason for hiding this comment

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

It's apparently 16KB for the buffer, so we'd be touching 4 pages on x86. That doesn't thrash all of a standard 32KB cache but it's still a lot when compared to simply zeroing a few shorts and longs.

otoh, "just tests".

@thomcc
Copy link
Member

thomcc commented Feb 11, 2023

Oof...

@bors r+

@bors
Copy link
Contributor

bors commented Feb 11, 2023

📌 Commit 59b11e8 has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2023
@ChrisDenton
Copy link
Member Author

Btw, as @clubby789 has said, the struct definition for REPARSE_MOUNTPOINT_DATA_BUFFER is actually very wrong. But things just so happen to line up enough that it works (at least for our usage). I guess it was reverse engineered at some point before the docs were made public.

I think I need to stop complaining about how bad this function is and actually do the rewrite already. I'll try some time this weekend. It would also be nice to add as a public API at some point.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2023
Rollup of 9 pull requests

Successful merges:

 - rust-lang#105019 (Add parentheses properly for borrowing suggestion)
 - rust-lang#106001 (Stop at the first `NULL` argument when iterating `argv`)
 - rust-lang#107098 (Suggest function call on pattern type mismatch)
 - rust-lang#107490 (rustdoc: remove inconsistently-present sidebar tooltips)
 - rust-lang#107855 (Add a couple random projection tests for new solver)
 - rust-lang#107857 (Add ui test for implementation on projection)
 - rust-lang#107878 (Clarify `new_size` for realloc means bytes)
 - rust-lang#107888 (revert rust-lang#107074, add regression test)
 - rust-lang#107900 (Zero the `REPARSE_MOUNTPOINT_DATA_BUFFER` header)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a50c379 into rust-lang:master Feb 11, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 11, 2023
@ChrisDenton ChrisDenton deleted the zero-header branch February 11, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Windows' symlink_junction_inner mysteriously fails
6 participants