Repository navigation
Conversation
This commit performs the unenviable task of implementing directory traversal for Windows, Unix-like platforms and Linux (where Linux is supported via the generic Unix implementation, but also has its own specialized implementation that uses getdents64 directly). The purpose of doing this is largely to control our costs more explicitly, and to fix platform specific bugs. For costs, this largely boils down to amortizing allocation. That is, instead of allocating a fresh OsString for every entry, we can now read directory entries into a previously used entry. Anecdotally, this leads to a 20-30% performance improvement on listing a single large directory for *both* Windows and Linux. As for bugs, these mostly center around long file paths. For example, on Unix, the typical maximum path length is 4096 bytes. There is no way to avoid this using std's APIs, so we need to instead roll our own based on file descriptors. There's also Windows, which has a maximum file path length of only 260 chars, although this is trickier to fix. There are basically two downsides to doing things this way: 1. This increases maintenance burden quite a bit, and this change will almost certainly introduce bugs on less well tested platforms. 2. It's not clear yet whether we will *also* need to hand-roll our own `Metadata` implementation. std provides no way to build the `Metadata` type from raw inputs. The key factor here is that std's `DirEntry::metadata` method benefits from using `fstatat` internally. If we don't re-roll our own `Metadata`, then we'll have to use `std::fs::metadata`, which does a normal `stat` call on the file path. `fstatat` should generally be faster. On the other hand, std's implementation strategy also means that file descriptors aren't closed until the last `DirEntry` is dropped, which is a bit sneaky. (And is also indicative of a bug in walkdir, since this means its "maximum opened fds" feature doesn't actually work.) Unfortunate, but everything gets sacrificed at the alter of performance.
This moves the DirEntry and Error types out into their own separate modules. This is prep work to (hopefully) make the impending refactoring (or more likely, rewrite) more palatable.
|
Bumping MSRV is fine. I have to ask: is the PR summary AI generated? If so, please write it in your own words. Same for commit messages. |
|
Thanks, @amaanq. I hope when this is done, we will have some valuable experience to inform |
|
I took a quick skim here, and this PR is unfortunately really large. Or more precisely, each individual commit is very large. I'm fine with a big PR since I think it's necessary in this case, but breaking subsequent changes down on top of Some statement about what is done with an LLM vs not would be helpful. But like, there are parts of this PR that stick out to me as sloppy. For example, this repo doesn't use Clippy, yet there are |
The `ag/sys` branch leaves the new `WalkDir` implementation unfinished. This brings back the existing path-based traversal without using the new platform APIs yet. This gives the later commits a working baseline.
The platform directory readers cannot construct `std::fs::FileType`. We introduce `walkdir::FileType` while entries still come from `std`. This keeps the public API change separate from changing traversal.
This switches supported Unix targets to `getdents` or `readdir`. Directory opens are still path based in this commit. Other Unix targets keep using `std::fs::ReadDir`. The build script selects the backend from `CARGO_CFG_TARGET_OS`, which also makes DragonFly cross builds use the correct `errno` shim. Temporary `libc` and `cc` upper bounds keep this part of the stack working on Rust 1.60. They are removed when the MSRV is bumped later.
Opening every child by its full path leaves traversal subject to `PATH_MAX` and symlink swaps. We fix this by keeping each active directory fd open and using `openat` for its children. `O_NOFOLLOW` is used when links are disabled. Sorting and `max_open` can discard the live parent handle. In those cases, traversal still falls back to the full path.
`FindFirstFile` still relies on wildcard paths and gives us metadata that cannot be turned back into `std::fs::Metadata`. We replace it with `FILE_ID_BOTH_DIR_INFO` enumeration on an open directory handle. `DirEntry::metadata` now stats on demand, as it does on Unix. Child directories are still opened by path in this commit. This is where `windows-sys` enters, so bump the MSRV to Rust 1.71 and remove the temporary dependency bounds.
This switches Windows descent to `NtOpenFile` using the active parent handle. We request `FILE_OPEN_REPARSE_POINT` when links are disabled so a swapped final component is not followed. Malformed or oversized UTF-16 names are rejected before constructing a `UNICODE_STRING`.
Only the directory at the top of the stack needs its parent path. We move a single `PathBuf` with the stack instead of cloning one into every `DirList`. This removes one allocation for each directory descent.
Yeah, of course. A good amount of the code was LLM generated, but I was in the loop the entire time & reviewed or edited all of it. As for the clippy allows, that was actually entirely done by me on purpose, since they stuck out on certain architectures only. Sorry about that :) it's just a force of habit to always run cargo clippy. I've restructured the commits to be a bit smaller individually, so reviewing the PR commit by commit would be the way to go. Initially, the commit bodies were llm generated before, and the pr description, albeit edited by me. I've gone ahead and redone them myself now, and I apologize about not being upfront on the LLM usage right away. I look forward to your review and feedback |
|
Also another question for you. Since the original branch ended with the recursive walker unfinished, my first commit restored the path-based traversal to get back to a working baseline and then the later commits replace that code piece by piece. Technically there could be less churn here by directly editing your commit beforehand to not be in a broken state. If you would like, I could fold the restoration into the |
|
@BurntSushi How is this now? In particular, what do you think of the "incrementally build up and then sudden switchover" strategy? It is not as good as incrementally introducing code that is live the entire time, of course, but we couldn't think of an alternative that still allowed for the commits to be smaller. |
|
I honestly don't know. I have to find time to review this and it might take quite some time. I agree this is probably the right way to go about it. |
|
Hello, Maintainer of async-walkdir here. While running a security code review with opus-5-5 on the crate I went into the TOCTOU issue and implemented a fix for it following a similar approach as yours (see ririsoft/async-walkdir#22) using opus-5-5 with adversarial review from GPT-5.6-Sol. Whether my LLM run got inspiration from your PR, or LLMs got it that way already is left open for the debate :) ... I went for optional and experimental opt-in of a secured implementation for Just wanted to share the way I handled it in case it can help your here. Note : I am not a big fan of maintaining my async-walkdir crate, and I believe most users can make their own with the sync WalkDir, to limit their dependencies, but as I receive issues or PRs from time to time, I keep maintain it for a while ... |
4 participants
Problem
The
ag/sysbranch added the platform-specific directory readers, butnever finished hooking them into recursive traversal. Child directories
were still opened from their full path, which is where the long-path
and TOCTOU problems come from.
Solution
I've finished the walker so that it keeps each active parent directory
open and opens its children relative to it. On Unix, this uses
openat, andon Windows this uses
NtOpenFile.Directory entries are read with
getdentsorreaddiron supportedUnix targets, and with
GetFileInformationByHandleExon Windows. On other Unixtargets, we fall back to
std::fs::ReadDir. Note that sorting andmax_opencandiscard the parent handle, so those cases still fall back to opening the full path.
The changes I've added are split into seven commits. First, I restored the
path-based walker and added
walkdir::FileType. Unix enumeration andrelative opens come next as separate commits, followed by the equivalent
Windows changes. The last commit just reuses the parent path buffer.
I also dropped
BorrowedIterand its allocation benchmark. They aren'tneeded for this change, but if you'd like them back please let me know and I'd
be happy to restore that.
Notes
There are a couple of changes worth pointing out. One is that
DirEntry::file_type()now returns
walkdir::FileType, since the platform readers cannot constructstd::fs::FileType.Secondly,
DirEntry::metadata()on Windows now stats the entrywhen called instead of returning metadata saved during enumeration.
Last, but not least, the MSRV is now Rust 1.71 because of
windows-sys,which is used because
winapidoes not expose theNt*APIs used here.LLM use
I used Codex (namely 5.5/5.6 sol) for the seven commits after that. They helped
mostly with code exploration and the implementation, though reviewing & testing
the code was done by me.
Testing
I've tested this on Linux, Windows on both gnu and msvc, FreeBSD, OpenBSD,
NetBSD, DragonFly, Solaris and macOS. (Thank you to cfarm :) )
(cc @Ericson2314)