std: use readdir on nearly all UNIX platforms - #158727
Conversation
|
cc @rust-lang/miri |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
@joboet - you've linked to VxWorks docs under the QNX entry? https://www.qnx.com/developers/docs/8.0/com.qnx.doc.neutrino.lib_ref/topic/r/readdir.html says:
https://www.qnx.com/developers/docs/7.1/com.qnx.doc.neutrino.lib_ref/topic/r/readdir.html says the same thing |
|
Whoops, I got the RTOSs confused 😉 ... thanks for the link! |
@joboet I completely agree with your analysis that FAT I've filed the issue upstream. Thanks a lot for spotting this! EDIT: I haven't put an effort to actually reproduce it on real hardware yet; only static analysis (confirmed by Fable though). So still a slim chance we might be missing something. Very slim. :-) |
|
Per today's t-libs meeting, we like this, +1. The possibility was also raised for those other platforms of invoking the underlying syscall directly if they're stable, but that's probably a question for a different PR. Thanks ^^ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@JohnTitor reminder that this is waiting for review :) |
Co-authored-by: Ralf Jung <post@ralfj.de>
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ |
std: use `readdir` on nearly all UNIX platforms POSIX.1-2024 [formalised](https://pubs.opengroup.org/onlinepubs/9799919799/functions/readdir.html) what was already guaranteed by a lot of implementations and required `readdir` to be thread-safe as long as an individual `DIR*` is not accessed concurrently (which `ReadDir::next` ensures by taking a mutable reference). But since our `read_dir` implementation predates that standard, we currently only utilise `readdir` on the platforms that guarantee thread-safety in their documentation. On other implementations – notably macOS – we use the `readdir_r` function, which was always required to be thread-safe but is problematic because it cannot handle directory entries with names longer than `NAME_MAX`. However, even the first POSIX issue, POSIX.1-1994, specified that the data in the returned `dirent` > is not overwritten by another call to readdir() on a different directory stream. and that guarantee together with the requirement that the underlying syscalls need to be thread-safe already because of `readdir_r` result in `readdir` being thread-safe on nearly all implementations, even if they predate POSIX.1-2024. Given the now formalised guarantee I think it safe to assume that currently thread-safe implementations will not be changed in a way that violates thread-safety. CC T-libs, do you agree? @rustbot label +I-libs-nominated I thus looked at the `readdir` implementation of all the UNIXes currently utilising `readdir_r` to check for thread-safety: - [x] [Apple platforms](https://github.com/apple-oss-distributions/Libc/blob/8a5571058ea8cf099279d8df1602958327b1d400/gen.subproj/readdir.c), starting with Mac OS X 10.0. While the original implementation did not use any synchronisation, it only modifies data of the `DIR*` it acts on. - [x] [Cygwin](https://github.com/cygwin/cygwin/blob/854594be786023d7e92c9352d8f54e64f9fb36eb/winsup/cygwin/dir.cc): same thing. - [x] [Dragonfly](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/2e3a87ce41bfdf759c1936e31cd8cb7be0a9c1cc/lib/libc/gen/readdir.c) uses locks. - [x] [L4Re](https://github.com/kernkonzept/l4re-core/blob/b6f7495bb8b010c6d0f2d44a25c61885a6ac7ac1/libc/musl/contrib/musl/src/dirent/readdir.c): only modifies the passed `DIR*`. - [ ] LynxOS: closed-source, the [documentation](https://www-f9.ijs.si/~rok/detectors/doc/LynxOS-2.5.0/LynxOS_Documentation.html) says that `readdir` is not reentrant. CC @rfatykhov-lynx - [x] [Managarm](https://github.com/managarm/mlibc/blob/368a00fa3ab482a76fbc2fb04afc188c6ff2407b/options/posix/generic/dirent.cpp): only modifies the passed `DIR*`. - [x] [NetBSD](https://github.com/NetBSD/src/blob/3604f3d8178e005847b7c7a1c4e4fa20838cecb8/lib/libc/gen/readdir.c): either uses locks depending on configuration, or only modifies the passed `DIR* `. - [x] [OpenBSD](https://github.com/openbsd/src/blob/394336142320c7c38e2361c4293df4c86626668e/lib/libc/gen/readdir.c): uses locks. - [x] [Unikraft](https://github.com/unikraft/unikraft/blob/be744898b6947824e367e01765703401e08ce3c5/lib/nolibc/musl-imported/src/dirent/readdir.c) - [ ] VxWorks: closed-source, the [documentation](https://archive.org/details/manualzilla-id-5786163) does not make any reference to thread-safety. CC @biabbas @hax0kartik - [x] QNX: the [documentation](https://www.qnx.com/developers/docs/8.0/com.qnx.doc.neutrino.lib_ref/topic/r/readdir.html) gives the same guarantee as POSIX.1-2024. - [x] [NuttX](https://github.com/apache/nuttx/blob/9a4114a9d3e06e47cab2c15e00b2b29c17c10b10/libs/libc/dirent/lib_readdir.c): only modifies the passed `DIR*`. - [x] 3DS: the underlying [directory read implementation in libctru](https://github.com/devkitPro/libctru/blob/36fe1ada5b7ebe53ba4decda36d764a55f8fefb6/libctru/source/services/fs.c#L1835-L1850) is thread-safe - [ ] RTEMS: I'm unable to find the implementation. CC @thesummer - [ ] QuRT: CC @androm3da - [ ] ESP-IDF: `readdir` is *not* thread-safe (at least on FAT) due to [caching file metadata in the VFS context without locks](https://github.com/espressif/esp-idf/blob/fa8039b5cadb6e85dd830ff8c2c4bd73b6538aee/components/fatfs/vfs/vfs_fat.c#L976). [`opendir`](https://github.com/espressif/esp-idf/blob/fa8039b5cadb6e85dd830ff8c2c4bd73b6538aee/components/fatfs/vfs/vfs_fat.c#L924) does the same thing, too?! And the cache is never invalidated, even when the file is deleted?! Honestly, this is just broken... CC @ivmarkov @MabezDev @SergioGasquez - [x] [Emscripten](https://github.com/emscripten-core/emscripten/blob/f351c42755ad56323f0bb3b2195ca50e35650496/system/lib/libc/musl/src/dirent/readdir.c): derived from musl, only modifies the current `DIR*`. On the implementations where I couldn't confirm thread-safety `ReadDir` will still use `readdir_r`, but I've changed the code so that this edge-case is limited to `ReadDir::next`. All other platforms now use `readdir`.
…uwer Rollup of 26 pull requests Successful merges: - #153749 (Account for ownership mismatch on argument that doesn't meet bound) - #158727 (std: use `readdir` on nearly all UNIX platforms) - #159130 (a bit optimize four-digit chunks in integer formatting) - #159326 (Deny multiple EII impls on a single item) - #159535 (Optimize slice::contains for bytewise types) - #159595 (Promote loongarch32-unknown-none* to Tier 2) - #160184 (Add -Zinstrument-mcount={fentry-nop-record,fentry-record}) - #160320 (point at trait definition when it is used as a derive macro) - #160369 (When suggesting method names, prefer *exact* doc aliases over similar names) - #160406 (`DepKind` cleanups) - #160424 (Use `thread::available_parallelism` as the default limit for backend parallelism) - #159303 (Fix ICE for direct inline const generic defaults) - #159977 (Add regression test for bool indexing codegen) - #160011 (remove InterpError::map_err_info) - #160165 (reject `...` without pattern post-expansion) - #160295 (Fix rustdoc ICE when checking if a generic arg can be elided) - #160305 (Linkify C-SKY targets in `platform-support.md`) - #160314 (fix borrowck ICE for consts with fn pointer type) - #160322 (ElaborateBoxDeref: remove unnecessary projection) - #160338 (Add regression test for supertrait associated type normalization through dyn) - #160340 (Add regression test for unused_parens on contract clauses) - #160371 (Add doc aliases for transpositions `read_exact_buf` and `read_exact_buf_at`) - #160384 (Add PR body notes for Cargo lock file maintenance) - #160412 (Move duplicate-names check for #[rustc_must_implement_one_of] to attribute parser) - #160435 (bump tracing-tree) - #160449 (Fix lookup of object files)
|
Possibly failed in rollup? #160469 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#160469), which was unapproved. |
This comment has been minimized.
This comment has been minimized.
std: use `readdir` on nearly all UNIX platforms try-job: x86_64-msvc-ext2
|
💔 Test for 8471650 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
Apparently Miri tests are failing on x86_64-apple-darwin. PR CI passed, meaning aarch64-apple-darwin seems okay... I did not realize that those targets sometimes use different symbols. |
Oh yeah, I forgot about those... the |
View all comments
POSIX.1-2024 formalised what was already guaranteed by a lot of implementations and required
readdirto be thread-safe as long as an individualDIR*is not accessed concurrently (whichReadDir::nextensures by taking a mutable reference). But since ourread_dirimplementation predates that standard, we currently only utilisereaddiron the platforms that guarantee thread-safety in their documentation. On other implementations – notably macOS – we use thereaddir_rfunction, which was always required to be thread-safe but is problematic because it cannot handle directory entries with names longer thanNAME_MAX.However, even the first POSIX issue, POSIX.1-1994, specified that the data in the returned
direntand that guarantee together with the requirement that the underlying syscalls need to be thread-safe already because of
readdir_rresult inreaddirbeing thread-safe on nearly all implementations, even if they predate POSIX.1-2024. Given the now formalised guarantee I think it safe to assume that currently thread-safe implementations will not be changed in a way that violates thread-safety.CC T-libs, do you agree?
@rustbot label +I-libs-nominated
I thus looked at the
readdirimplementation of all the UNIXes currently utilisingreaddir_rto check for thread-safety:DIR*it acts on.DIR*.readdiris not reentrant. CC @rfatykhov-lynxDIR*.DIR*.DIR*.readdiris not thread-safe (at least on FAT) due to caching file metadata in the VFS context without locks.opendirdoes the same thing, too?! And the cache is never invalidated, even when the file is deleted?! Honestly, this is just broken... CC @ivmarkov @MabezDev @SergioGasquezDIR*.On the implementations where I couldn't confirm thread-safety
ReadDirwill still usereaddir_r, but I've changed the code so that this edge-case is limited toReadDir::next. All other platforms now usereaddir.