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

Implement a readdir64() shim for Linux #1981

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Conversation

tavianator
Copy link
Contributor

Partial fix for #1966.

@tavianator
Copy link
Contributor Author

This doesn't quite work yet, but I think I need some help to fix it. I'm allocating the struct dirent as small as possible, in order to catch bugs like rust-lang/rust#93384 (comment). The pointer returned by readdir() may point to a region smaller than sizeof(struct dirent).

My libstd patch was supposed to handle this by using ptr::addr_of!(). And yet I get this from miri:

error: Undefined Behavior: dereferencing pointer failed: alloc1498 has size 24, so pointer to 280 bytes starting at offset 0 is out-of-bounds
   --> /home/tavianator/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/unix/fs.rs:495:34
    |
495 |                 let entry_name = ptr::addr_of!((*entry_ptr).d_name) as *const u8;
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dereferencing pointer failed: alloc1498 has size 24, so pointer to 280 bytes starting at offset 0 is out-of-bounds
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
            
    = note: inside `<std::sys::unix::fs::ReadDir as std::iter::Iterator>::next` at /home/tavianator/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1523:5
    = note: inside `<std::fs::ReadDir as std::iter::Iterator>::next` at /home/tavianator/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/fs.rs:1411:9
note: inside `main` at dir.rs:2:18
   --> dir.rs:2:18
    |
2   |     for entry in std::fs::read_dir(".").unwrap() {
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `ptr::addr_of` (in Nightly builds, run with -Z macro-backtrace for more info)

@tavianator
Copy link
Contributor Author

Hmm I guess this is just a libstd bug. addr_of!((*ptr).field) requires ptr to point to a properly sized/aligned region: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=904126751c84698fc38c8dde790de358. I'll patch libstd again :P

@RalfJung
Copy link
Member

addr_of!((*ptr).field) requires ptr to point to a properly sized/aligned region

Indeed, *ptr asserts that ptr is dereferencable (for the size and alignment determined by its type).

src/shims/posix/fs.rs Outdated Show resolved Hide resolved
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

That's great, thanks a lot. :) I hadn't even thought of creating a new allocation for each call, but it makes a lot of sense.

@saethlin
Copy link
Member

I'll patch libstd again :P

Another one for the trophy case? :P

@RalfJung
Copy link
Member

I guess we could add it. ;)

tavianator added a commit to tavianator/rust that referenced this pull request Feb 23, 2022
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)
@RalfJung RalfJung added the S-blocked-on-rust Status: Blocked on landing a Rust PR label Feb 24, 2022
@bors
Copy link
Contributor

bors commented Mar 5, 2022

☔ The latest upstream changes (presumably #1970) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2022
…=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)
@tavianator tavianator mentioned this pull request Mar 7, 2022
@RalfJung RalfJung removed the S-blocked-on-rust Status: Blocked on landing a Rust PR label Mar 7, 2022
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 7, 2022

☔ The latest upstream changes (presumably #2005) made this pull request unmergeable. Please resolve the merge conflicts.

src/machine.rs Outdated Show resolved Hide resolved
In preparation to use it for other runtime-internal allocations.
@RalfJung
Copy link
Member

RalfJung commented Mar 7, 2022

(Looks at the force-push diff, confused) oh, you rebased! That makes force-push diffs harder to read so please only do it when needed to resolve conflicts. :)

@RalfJung
Copy link
Member

RalfJung commented Mar 7, 2022

Looking great, thanks a lot for staying with me through the many rounds of review. :)

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2022

📌 Commit 0886419 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 7, 2022

⌛ Testing commit 0886419 with merge fc15f96...

@bors
Copy link
Contributor

bors commented Mar 7, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing fc15f96 to master...

@bors bors merged commit fc15f96 into rust-lang:master Mar 7, 2022
@tavianator tavianator deleted the readdir branch March 12, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants