rust
rust copied to clipboard
rust-lang
Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`)
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) some time within the next two weeks.
Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:
-
@rustbot author: the review is finished, PR author should check the comments and take action accordingly -
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue
Commands to the bots go in the PR description, not the title. Please prefer concise and descriptive PR titles.
r? @saethlin
The PR description is misleading. It does not really fix the soundness issue. And it's clearly motivated by compatibility with a buggy or incomplete filesystem implementation. I'm not a fan, but at least the PR should be honest about what's doing.
@the8472 My choice in direction to update the title was based on my interpretation of @saethlin's remarks here that solicited the PR:
- https://github.com/rust-lang/rust/issues/122262#issuecomment-1987048697
- https://github.com/rust-lang/rust/issues/122262#issuecomment-1987053767
Perhaps I have misinterpreted @saethlin's remarks, but in any case, I only sought to choose something marginally preferable to the first draft when I edited it. I will leave further edits to the contributor, but please be more precise than "doesn't really fix".
As saethlin said:
So if anything, MAP_PRIVATE is better because it's possible that a modification to the file wouldn't update the mapping and cause UB.
The distinction between MAP_PRIVATE and MAP_SHARED only comes into play when the private mapping is touched with write-intent, only then the CoW mapping becomes unshared. If other processes write to it through write syscalls or through a shared mapping then the CoW will not be broken. To my knowledge rustc never does touch those pages with write intent.
So while in the broad abstract private mappings can reduce the possibility of writes from other processes becoming visible that is not relevant to the way rust is using it since we only take immutable borrows from those mappings.
Which in turn means if there hypothetically were another process corrupting the files we'd still see immutable data change under our noses. So the UB is still there as it ever was, but this has already been deemed outside-rustc-robustness-scope in previous discussions.
Based on the linked issue the PR aims to do is to change the syscall flag to accommodate a filesystem that doesn't offer full posix support and this is not reflected in the description or the code comments. (Edit) The commit message seems more appropriate, though a bit terse.
The PR description is misleading. It does not really fix the soundness issue. And it's clearly motivated by compatibility with a buggy or incomplete filesystem implementation. I'm not a fan, but at least the PR should be honest about what's doing.
@the8472 If you take issue with the PR title (I don't see how a reasonable person could take issue with the description) please suggest an alternative instead of being vague with a first-time contributor who is still learning about the Rust concept of soundness.
Yes, I mean the title. And it was modified by jubilee. The previous content was better, apart from the bot command.
@the8472 Given the breadth of what "virtualization" includes, including many subtly and not-so-subtly varying goals and methods to achieve it, that data point seemed pointlessly vague to me. It's like opening an LLVM bump PR and saying it's "for multiplication". I suppose that is technically true? In practice it is misleadingly vague, so it remains surprising to me that you would say anything was "clearly" anything.
Hey folks, please focus on the PR contents at hand and be constructive and helpful when commenting.
:pushpin: Commit 3fc5ed8067f98e2cae8c259a2e367024c59a3ad3 has been approved by saethlin
It is now in the queue for this repository.
:hourglass: Testing commit 3fc5ed8067f98e2cae8c259a2e367024c59a3ad3 with merge 774ae599aba35f7b323e77be88923b8f476ee783...
:sunny: Test successful - checks-actions Approved by: saethlin Pushing 774ae599aba35f7b323e77be88923b8f476ee783 to master...
Finished benchmarking commit (774ae599aba35f7b323e77be88923b8f476ee783): comparison URL.
Overall result: no relevant changes - no action needed
@rustbot label: -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) |
4.9% | [4.9%, 4.9%] | 1 |
| Regressions ❌ (secondary) |
2.8% | [2.8%, 2.8%] | 1 |
| Improvements ✅ (primary) |
-4.0% | [-4.9%, -3.1%] | 2 |
| Improvements ✅ (secondary) |
-2.3% | [-3.5%, -0.5%] | 9 |
| All ❌✅ (primary) | -1.0% | [-4.9%, 4.9%] | 3 |
Cycles
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) |
- | - | 0 |
| Regressions ❌ (secondary) |
1.2% | [1.2%, 1.2%] | 1 |
| Improvements ✅ (primary) |
- | - | 0 |
| Improvements ✅ (secondary) |
- | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 671.724s -> 671.684s (-0.01%) Artifact size: 311.58 MiB -> 311.57 MiB (-0.00%)
