fs: Don't copy d_name from struct dirent #93459

tavianator · 2022-01-29T16:26:25Z

The dirent returned from readdir() is only guaranteed to be valid for
d_reclen bytes on common platforms. Since we copy the name separately
anyway, we can copy everything except d_name into DirEntry::entry.

Fixes #93384.

rust-highfive · 2022-01-29T16:26:28Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

tavianator · 2022-01-29T17:19:45Z

I added a test, and was able to reproduce the failure with

$ RUSTFLAGS_BOOTSTRAP="-Z sanitizer=address" ./x.py test --stage 0 library/std --test-args read_large_dir

and confirmed that this PR fixes it.

library/std/src/sys/unix/fs.rs

cuviper · 2022-01-29T20:16:32Z

library/std/src/sys/unix/fs.rs

+                // Only d_reclen bytes of *entry_ptr are valid, so we can't just copy the
+                // whole thing (#93384).  Instead, copy everything except the name.
+                let entry_bytes = entry_ptr as *const u8;
+                let entry_name = (*entry_ptr).d_name.as_ptr() as *const u8;


The reason I suggested ptr::addr_of! is because things like as_ptr() require forming a reference &self, but our issue here is that &d_name may not be fully dereferenceable. Keeping everything as pointers should be safer.

Ah fair, I did it this way because the code below was already doing the same thing. I'll change it.

Turns out addr_of!() isn't even enough: rust-lang/miri#1981 (comment)

Ugh, then what? Get the offset from a fully-sized local dummy, then ptr-add on the real thing?

Yeah. That's apparently what all the offset_of!() crates do. Does libstd already have something like that or should I just open code it?

My impression was that such use was the very purpose of addr_of! / &raw, and it seems I wasn't the only one from the discussion on #64490 and #73394. But that was left to the future in RFC 2582.

It seems really unfortunate here since the pointer at hand is not entirely dangling, just incomplete at the end, beyond the pointer we want to compute. A MaybeUninit local feels like such a waste, but should be optimized away. Maybe that can even be done in a true const these days?

I'm not aware of any existing offset_of!, so open-coding it seems fine, with a comment that it could use offset_of! so we can find and update it later. :)

addr_of! is a weaker from of &, but it does nothing about *. The code here is UB due to the requirement of *ptr to only be used on pointers that point to actual memory (with the required size and alignment given by its type).

I would love to just entirely remove that restriction, but then we would have to change our LLVM codegen to use getelementptr instead of getelementptr inbounds in some situations.

Maybe that can even be done in a true const these days?

With some unstable features, it can -- see the https://github.com/Gilnaa/memoffset crate

I would love to just entirely remove that restriction, but then we would have to change our LLVM codegen to use getelementptr instead of getelementptr inbounds in some situations.

This is probably worth doing, honestly. I can't imagine the use of gepi over gep wins us almost anything on modern hardware (IIUC it mostly just allows folding the address arithmetic into a subsequent load), and even if it does, we still would be able to use it in all cases except for with addr_of.

Cc rust-lang/unsafe-code-guidelines#319

cuviper · 2022-01-29T20:21:20Z

r? @cuviper

The dirent returned from readdir() is only guaranteed to be valid for d_reclen bytes on common platforms. Since we copy the name separately anyway, we can copy everything except d_name into DirEntry::entry. Fixes rust-lang#93384.

cuviper · 2022-01-29T22:22:49Z

LGTM -- bumping priority since this helps everyone's CI.

@bors r+ p=1

bors · 2022-01-29T22:22:51Z

📌 Commit d0c8b29 has been approved by cuviper

bors · 2022-01-30T04:11:17Z

⌛ Testing commit d0c8b29 with merge c7a67c9e4bd271310a9b6a163aa41c82ccf863ed...

bors · 2022-01-30T04:25:18Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-01-30T04:25:20Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

ehuss · 2022-01-30T06:40:43Z

@bors retry

The build was canceled after 10 minutes, though it isn't clear why.

…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#93256 (Make `join!` description more accurate) - rust-lang#93358 (Add note suggesting that predicate may be satisfied, but is not `const`) - rust-lang#93362 (Do not register infer var for GAT projection in RPIT) - rust-lang#93391 (rustdoc: remove tooltip from source link) - rust-lang#93414 (Move unstable is_{arch}_feature_detected! macros to std::arch) - rust-lang#93441 (rustdoc: load the set of in-scope traits for modules with no docstring) - rust-lang#93459 (fs: Don't copy d_name from struct dirent) - rust-lang#93463 (Rename _args -> args in format_args expansion) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2022-01-30T13:38:15Z

⌛ Testing commit d0c8b29 with merge 7cc28c1...

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)

…=cuviper fs: Don't dereference a pointer to a too-small allocation 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)

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

rust-highfive assigned kennytm Jan 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2022

tmiasko reviewed Jan 29, 2022

View reviewed changes

library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved

tavianator force-pushed the dirent-copy-only-reclen branch from 4f0de2b to e8690d8 Compare January 29, 2022 19:21

tavianator changed the title ~~fs: Only copy d_reclen bytes of struct dirent~~ fs: Don't copy d_name from struct dirent Jan 29, 2022

tavianator force-pushed the dirent-copy-only-reclen branch from e8690d8 to db00028 Compare January 29, 2022 19:24

camelid added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 29, 2022

cuviper reviewed Jan 29, 2022

View reviewed changes

rust-highfive assigned cuviper and unassigned kennytm Jan 29, 2022

tavianator added 2 commits January 29, 2022 16:37

fs: Don't copy d_name from struct dirent

f8f4c40

The dirent returned from readdir() is only guaranteed to be valid for d_reclen bytes on common platforms. Since we copy the name separately anyway, we can copy everything except d_name into DirEntry::entry. Fixes rust-lang#93384.

fs: Add a regression test for rust-lang#93384

d0c8b29

tavianator force-pushed the dirent-copy-only-reclen branch from db00028 to d0c8b29 Compare January 29, 2022 21:37

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 Jan 29, 2022

cuviper mentioned this pull request Jan 29, 2022

Temporarily disable i686 mir-opt tests on PRs. #93447

Closed

matthiaskrgr mentioned this pull request Jan 29, 2022

Rollup of 8 pull requests #93468

Merged

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2022

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 Jan 30, 2022

bors merged commit 0d08bbc into rust-lang:master Jan 30, 2022

rustbot added this to the 1.60.0 milestone Jan 30, 2022

tavianator deleted the dirent-copy-only-reclen branch January 30, 2022 13:43

This was referenced Feb 22, 2022

Implement a readdir64() shim for Linux rust-lang/miri#1981

Merged

fs: Don't dereference a pointer to a too-small allocation #94272

Merged

RalfJung mentioned this pull request Feb 23, 2022

Should raw pointer deref/projections have to be in-bounds? rust-lang/unsafe-code-guidelines#319

Closed

dtolnay mentioned this pull request Oct 17, 2022

Eliminate 280-byte memset from ReadDir iterator #103137

Merged

cuviper mentioned this pull request Apr 5, 2023

[strict provenance] Fix System APIs That Are Liars #95496

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: Don't copy d_name from struct dirent #93459

fs: Don't copy d_name from struct dirent #93459

tavianator commented Jan 29, 2022 •

edited

Loading

rust-highfive commented Jan 29, 2022

tavianator commented Jan 29, 2022

cuviper Jan 29, 2022 •

edited

Loading

tavianator Jan 29, 2022

tavianator Feb 22, 2022

cuviper Feb 22, 2022

tavianator Feb 22, 2022

cuviper Feb 22, 2022

RalfJung Feb 23, 2022 •

edited

Loading

RalfJung Feb 23, 2022

thomcc Feb 23, 2022

RalfJung Feb 23, 2022

cuviper commented Jan 29, 2022

cuviper commented Jan 29, 2022

bors commented Jan 29, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

rust-log-analyzer commented Jan 30, 2022

ehuss commented Jan 30, 2022

bors commented Jan 30, 2022

fs: Don't copy d_name from struct dirent #93459

fs: Don't copy d_name from struct dirent #93459

Conversation

tavianator commented Jan 29, 2022 • edited Loading

rust-highfive commented Jan 29, 2022

tavianator commented Jan 29, 2022

cuviper Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Jan 29, 2022

cuviper commented Jan 29, 2022

bors commented Jan 29, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

rust-log-analyzer commented Jan 30, 2022

ehuss commented Jan 30, 2022

bors commented Jan 30, 2022

tavianator commented Jan 29, 2022 •

edited

Loading

cuviper Jan 29, 2022 •

edited

Loading

RalfJung Feb 23, 2022 •

edited

Loading