dirfd file operations (2/4) by Qelxiros · Pull Request #150679 · rust-lang/rust

Qelxiros · 2026-01-04T18:13:00Z

Previous PR: #146341
Reference: #139514
Tracking issue: #120426

rustbot · 2026-01-04T18:13:05Z

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Qelxiros · 2026-01-04T21:31:36Z

This is going to need a try run for windows at least. @tgross35 I'll let you decide which jobs make the most sense here.

tgross35 · 2026-01-04T23:25:01Z

@bors2 try jobs=x86_64-msvc-*

Cc @the8472 for dirfd, and @ChrisDenton for Windows

dirfd file operations (2/4) try-job: x86_64-msvc-*

rust-bors · 2026-01-05T02:06:13Z

☀️ Try build successful (CI)
Build commit: 796e04e (796e04e9c306cfa50aa215b1e93ebdb1cbdc51d8, parent: e29fcf45e4ae686d77b490bf07320f0d3a2cf35f)

tgross35 · 2026-01-24T05:12:47Z

I haven't had a chance to look here yet, sorry

@rustbot reroll

Qelxiros · 2026-04-01T23:05:13Z

@rustbot reroll

Mark-Simulacrum

The Windows code looks like it could use a second set of eyes from someone with Windows knowledge, unless we manage to remove most of the new code through reusing the existing rename logic.

View changes since this review

Mark-Simulacrum · 2026-04-04T13:21:23Z

+        opts.access_mode(c::DELETE);
+        opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT | c::FILE_FLAG_BACKUP_SEMANTICS);
+        let handle = self.open_file_native(from, &opts, dir)?;
+        // Calculate the layout of the `FILE_RENAME_INFORMATION` we pass to `NtSetInformationFile`


A bunch of the content here looks similar to

rust/library/std/src/sys/fs/windows.rs

Line 1313 in 2972b5e

let err = api::get_last_error();

-- could we reuse some of that? Is there an opportunity to use MoveFileExW for this path at all?

MoveFileExW operates exclusively on filenames. Dir::rename is intended to be TOCTOU-safe, so it needs to use directory Handles. I've factored out the NtSetInformationFile section of both functions into nt_rename. The implementations are a little different from one another, so I'm not 100% sure I got the function signature right. I'll wait for the CI to run and reevaluate.

tgross35 · 2026-05-27T20:10:20Z

@bors try jobs=x86_64-msvc-*

dirfd file operations (2/4) try-job: x86_64-msvc-*

rust-bors · 2026-05-27T23:02:38Z

☀️ Try build successful (CI)
Build commit: 88db364 (88db364c1a9f43a4eedde8ea04cc75e39e5dd9b5, parent: 77a4fb62f70c6ea05e1820216d903938e331d42b)

Mark-Simulacrum · 2026-05-31T14:46:29Z

r? ChrisDenton if you have a chance to take another look at the Windows code -- it looks reasonable to me at a glance though.

rustbot · 2026-05-31T14:46:33Z

ChrisDenton is not on the review rotation at the moment.
They may take a while to respond.

ChrisDenton

Windows code looks good to me, thanks!

View changes since this review

rustbot · 2026-06-07T00:58:22Z

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.

Qelxiros · 2026-06-07T02:32:50Z

@rustbot ready

Mark-Simulacrum · 2026-05-31T14:31:40Z

    handle: Handle,
 }

+fn run_path_with_u16s<T>(path: &Path, f: &dyn Fn(&[u16]) -> io::Result<T>) -> io::Result<T> {


Why do we need a callback-style interface here? I don't see why this can't be inlined into callers -- if we want to avoid duplicating the path.len() - 1 bit then that can be something like to_u16s_without_null or similar?

View changes since the review

Mark-Simulacrum · 2026-06-07T13:00:31Z

    }

+    pub fn remove_file(&self, path: &Path) -> io::Result<()> {
+        run_path_with_cstr(path, &|path| self.remove_c(path, false))


Should this check somewhere that the passed path is relative? Otherwise we're not actually getting the promised semantics: "If path is absolute, then dirfd is ignored." (https://www.man7.org/linux/man-pages/man2/unlink.2.html)

View changes since the review

That seems like a question for all dirfd-relative operations that take a path: should they require the path to be relative, or should they allow absolute paths and do the obvious thing for them? IMO it makes sense to allow absolute paths. That basically lets you use dirfd as a "local working directory" without the downsides of set_current_dir.

The argument against is that people would like to use dirfd in security relevant contexts. You can build a less secure variant on top of dirfd but not really the other way around (well technically you can but defaulting to the less secure option has historically been a footgun).

But in any case, I think this is a question for libs-api.

It's unstable, so you can put it on the open questions in the tracking issue? I don't think this needs to be settled immediately.

On linux there's openat2 which adds RESOLVE_BENEATH and RESOLVE_IN_ROOT flags which give you certain security properties, depending on which you want.

The tracking issue explicitly says that this is NOT using O_BENEATH. We also have a fallback implementation that just stores the dir name as a string. So anything security critical seems to be already ruled out.

I have added this to #120426.

rustbot · 2026-06-07T13:05:13Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot assigned tgross35 Jan 4, 2026

rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 4, 2026