fs: Don't dereference a pointer to a too-small allocation #94272
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Alternatively, we could deref it at a different, sufficiently small type -- not sure if that is feasible here. |
We could make a type that matches the layout of |
|
r? @cuviper |
ptr::addr_of!((*ptr).field) still requires ptr to point to an appropriate allocation for its type. Since the pointer returned by readdir() can be smaller than sizeof(struct dirent), we need to entirely avoid dereferencing it as that type. Link: rust-lang/miri#1981 (comment) Link: rust-lang#93459 (comment)
tavianator
force-pushed
the
readdir-reclen-for-real
branch
from
684e0ce to
478cf8b
Compare
|
@cuviper friendly reminder that this is blocking getting readdir to work again in Miri. :) |
|
Sorry, yeah, looks good. @bors r+ |
|
📌 Commit 478cf8b has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 478cf8b with merge 75640d6d8cc7bc15889d9c911fb337c1fe44a55c... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
I don't think the MSVC failure is related to this PR |
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (2631aee): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 23, 2022
Eliminate 280-byte memset from ReadDir iterator This guy: https://github.com/rust-lang/rust/blob/1536ab1b383f21b38f8d49230a2aecc51daffa3d/library/std/src/sys/unix/fs.rs#L589 It turns out `libc::dirent64` is quite big—https://docs.rs/libc/0.2.135/libc/struct.dirent64.html. In rust-lang#103135 this memset accounted for 0.9% of the runtime of iterating a big directory. Almost none of the big zeroed value is ever used. We memcpy a tiny prefix (19 bytes) into it, and then read just 9 bytes (`d_ino` and `d_type`) back out. We can read exactly those 9 bytes we need directly from the original entry_ptr instead. ## History This code got added in rust-lang#93459 and tweaked in rust-lang#94272 and rust-lang#94750. Prior to rust-lang#93459, there was no memset but a full 280 bytes were being copied from the entry_ptr. <table><tr><td>copy 280 bytes</td></tr></table> This was not legal because not all of those bytes might be initialized, or even allocated, depending on the length of the directory entry's name, leading to a segfault. That PR fixed the segfault by creating a new zeroed dirent64 and copying just the guaranteed initialized prefix into it. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td></tr></table> However this was still buggy because it used `addr_of!((*entry_ptr).d_name)`, which is considered UB by Miri in the case that the full extent of entry_ptr is not in bounds of the same allocation. (Arguably this shouldn't be a requirement, but here we are.) The UB got fixed by rust-lang#94272 by replacing `addr_of` with some pointer manipulation based on `offset_from`, but still fundamentally the same operation. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td></tr></table> Then rust-lang#94750 noticed that only 9 of those 19 bytes were even being used, so we could pick out only those 9 to put in the ReadDir value. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td><td>copy 9 bytes</td></tr></table> After my PR we just grab the 9 needed bytes directly from entry_ptr. <table><tr><td>copy 9 bytes</td></tr></table> The resulting code is more complex but I believe still worthwhile to land for the following reason. This is an extremely straightforward thing to accomplish in C and clearly libc assumes that; literally just `entry_ptr->d_name`. The extra work in comparison to accomplish it in Rust is not an example of any actual safety being provided by Rust. I believe it's useful to have uncovered that and think about what could be done in the standard library or language to support this obvious operation better. ## References - https://man7.org/linux/man-pages/man3/readdir.3.html
Aaron1011
pushed a commit
to Aaron1011/rust
that referenced
this pull request
Jan 6, 2023
Eliminate 280-byte memset from ReadDir iterator This guy: https://github.com/rust-lang/rust/blob/1536ab1b383f21b38f8d49230a2aecc51daffa3d/library/std/src/sys/unix/fs.rs#L589 It turns out `libc::dirent64` is quite big—https://docs.rs/libc/0.2.135/libc/struct.dirent64.html. In rust-lang#103135 this memset accounted for 0.9% of the runtime of iterating a big directory. Almost none of the big zeroed value is ever used. We memcpy a tiny prefix (19 bytes) into it, and then read just 9 bytes (`d_ino` and `d_type`) back out. We can read exactly those 9 bytes we need directly from the original entry_ptr instead. ## History This code got added in rust-lang#93459 and tweaked in rust-lang#94272 and rust-lang#94750. Prior to rust-lang#93459, there was no memset but a full 280 bytes were being copied from the entry_ptr. <table><tr><td>copy 280 bytes</td></tr></table> This was not legal because not all of those bytes might be initialized, or even allocated, depending on the length of the directory entry's name, leading to a segfault. That PR fixed the segfault by creating a new zeroed dirent64 and copying just the guaranteed initialized prefix into it. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td></tr></table> However this was still buggy because it used `addr_of!((*entry_ptr).d_name)`, which is considered UB by Miri in the case that the full extent of entry_ptr is not in bounds of the same allocation. (Arguably this shouldn't be a requirement, but here we are.) The UB got fixed by rust-lang#94272 by replacing `addr_of` with some pointer manipulation based on `offset_from`, but still fundamentally the same operation. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td></tr></table> Then rust-lang#94750 noticed that only 9 of those 19 bytes were even being used, so we could pick out only those 9 to put in the ReadDir value. <table><tr><td>memset 280 bytes</td><td>copy 19 bytes</td><td>copy 9 bytes</td></tr></table> After my PR we just grab the 9 needed bytes directly from entry_ptr. <table><tr><td>copy 9 bytes</td></tr></table> The resulting code is more complex but I believe still worthwhile to land for the following reason. This is an extremely straightforward thing to accomplish in C and clearly libc assumes that; literally just `entry_ptr->d_name`. The extra work in comparison to accomplish it in Rust is not an example of any actual safety being provided by Rust. I believe it's useful to have uncovered that and think about what could be done in the standard library or language to support this obvious operation better. ## References - https://man7.org/linux/man-pages/man3/readdir.3.html
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ptr::addr_of!((*ptr).field) still requires ptr to point to an
appropriate allocation for its type. Since the pointer returned by
readdir() can be smaller than sizeof(struct dirent), we need to entirely
avoid dereferencing it as that type.
Link: rust-lang/miri#1981 (comment)
Link: #93459 (comment)