Support getting file metadata on Windows #4067

CraftSpider · 2024-12-02T03:38:08Z

Main changes are the first two commits - one refactors handles a bit to make their usage more consistent and adds the file variant, the other actually adds the metadata shims.

This also changes miri to never return invalid handle errors, instead always treating them as an error. This behavior matches that of code running under the MSVC debugger - invalid handles are treated as exceptions in debug contexts.

Split off #4043

- consistent handling, invalid handles are always an abort - Helper for reading handles that does the checking and machine stop - Use real handle types more places - Adds `File` and `Invalid` types of handle. File support will be added next

… enough to get file metadata.

RalfJung · 2024-12-02T06:54:27Z

tests/pass/shims/fs.rs

Could you also add a test that directly calls the relevant Windows API functions? std sometimes changes what it uses and then we lose test coverge, which is not great.

Added a windows-fs test for the shims directly

RalfJung

Thanks! Here are some comments for the first and last commit, I skipped the second one for now.

src/shims/windows/handle.rs

tests/pass/shims/fs.rs

src/shims/windows/fs.rs

src/shims/windows/handle.rs

CraftSpider · 2024-12-06T04:47:22Z

Before we get too much further on this PR, I'd like to point out 296d806 (which I just pushed) on the big PR. It's the changes needed to FileDescription methods to make them platform-agnostic enough for Windows use-cases. If they look fine, we can continue, but if you think it's a bad design then the FileDescription trait will need entirely split in two, which will change code even in this PR.

RalfJung · 2024-12-06T06:57:58Z

Reducing the places where we have to use global state like errno seems like an improvement. But please use Result<_, IoError> (with Miri's IoError type).

CraftSpider · 2024-12-06T21:04:24Z

@rustbot ready

tests/pass/shims/windows-fs.rs

src/shims/windows/handle.rs

src/shims/windows/fs.rs

RalfJung · 2024-12-07T08:02:06Z

src/shims/windows/fs.rs

+    const TIME_TO_EPOCH: u64 = 31_556_926 * 369 * 10_000_000;
+    match time.ok() {
+        Some(time) => {
+            let duration = system_time_to_duration(&time)?;


We should probably rename this to system_time_since_unix_epoch. (Can be a future PR though.)

RalfJung · 2024-12-07T08:02:42Z

Okay I finally had a look at the core new file in this PR. :)
@rustbot author

tests/pass/shims/windows-fs.rs

…test for this case.

src/shims/windows/fs.rs

…nts to code about various quirks

…em in file APIs

CraftSpider · 2024-12-09T21:59:23Z

@rustbot ready

bors · 2024-12-12T08:08:12Z

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

RalfJung · 2024-12-12T16:48:53Z

src/shims/windows/fs.rs

+        if !(file_attribute_normal | file_flag_backup_semantics | file_flag_open_reparse_point)
+            & flags_and_attributes
+            != 0


This is another one of these bitmasks where validation is separate from interpretation. I don't understand all these flags so I am not sure, can this be done in a better way? There should be one if for each of these 3 cases, performing whatever action the respective flag should perform.

Maybe Windows file flag parsing is so complicated that we should have a separate function that takes in the raw flaws as u32s and returns some nice typed representation?

file_flag_backup_semantics is sus because it is considered supported here but entirely ignored later.

RalfJung · 2024-12-12T16:50:40Z

src/shims/windows/fs.rs

+            options.create_new(true);
+            // Per `create_new` documentation:
+            // If the specified file does not exist and is a valid path to a writable location, the
+            // function creates a file and the last-error code is set to zero.


The setting to zero does not seem to happen here?

This is the wrong copy-paste - I meant to put the std docs for create_new here. Microsoft doesn't say anything about zero error here

If you have access to a Windows host, maybe you can try this:

make sure the last error is non-zero

then call CreateFile in one of the cases where the docs say nothing about the last error, and see what the last error looks like when the function returns successfully

RalfJung · 2024-12-12T16:51:22Z

src/shims/windows/fs.rs

+            }
+            options.create(true);
+        } else if creation_disposition == open_existing {
+            // Nothing


Are you sure this does not reset the error code to 0?

Maybe we should just set it to 0 at the top, to make sure we don't forget?

RalfJung · 2024-12-12T16:52:07Z

src/shims/files.rs

@@ -275,6 +275,9 @@ impl VisitProvenance for WeakFileDescriptionRef {
    }
 }

+/// Internal type of a file-descriptor - this is what [`FdTable`] expects
+pub type FdNum = i32;


Please also use this type in the definition of FdTable

src/shims/windows/fs.rs

RalfJung · 2024-12-12T16:56:40Z

src/shims/windows/fs.rs

+        // This is racy, but there doesn't appear to be an std API that both succeeds if a file
+        // exists but tells us it isn't new. Either we accept racing one way or another, or we
+        // use an iffy heuristic like file creation time. This implementation prefers to fail in the
+        // direction of erroring more often.
+        let exists = file_name.exists();


This comment is too early, since one doesn't know yet that this is used for. IMO it'd be better placed at the if exists below.

(Also, no need to let-bind, I don't feel like this makes things much clearer.)

src/shims/windows/handle.rs

RalfJung · 2024-12-12T16:58:36Z

@rustbot author

RalfJung · 2024-12-16T07:06:16Z

src/shims/windows/fs.rs

+        /// This must be passed to allow getting directory handles. If not passed, we error on trying
+        /// to open directories
+        const BACKUP_SEMANTICS = 1 << 1;
+        const OPEN_REPARSE = 1 << 2;


Can you doc-comment this one as well? "reparse" is not standard terminology outside the Windows world.

RalfJung · 2024-12-16T07:07:06Z

src/shims/windows/fs.rs

+        if out == FileAttributes::ZERO {
+            out = FileAttributes::NORMAL;
+        }


Why is this done?

RalfJung · 2024-12-16T07:08:02Z

src/shims/windows/fs.rs

-                this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?;
-            } else {
-                this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?;
+            OpenExisting => (), // Nothing


Suggested change

OpenExisting => (), // Nothing

OpenExisting => {} // Default options

rustbot · 2024-12-29T09:26:12Z

☔ The latest upstream changes (possibly 887b498) made this pull request unmergeable. Please resolve the merge conflicts.

CraftSpider added 4 commits December 1, 2024 19:27

Refactor Handle slightly

9b61c05

- consistent handling, invalid handles are always an abort - Helper for reading handles that does the checking and machine stop - Use real handle types more places - Adds `File` and `Invalid` types of handle. File support will be added next

Implement trivial file operations - opening and closing handles. Just…

ab0e138

… enough to get file metadata.

Fix MAIN_THREAD handle in windows_join_main

e19deaa

Try fix for Windows paths on non-Windows machines

f1bb77d

CraftSpider mentioned this pull request Dec 2, 2024

Windows file system shims are missing #3482

Open

RalfJung reviewed Dec 2, 2024

View reviewed changes

RalfJung reviewed Dec 3, 2024

View reviewed changes

RalfJung reviewed Dec 5, 2024

View reviewed changes

src/shims/windows/handle.rs Outdated Show resolved Hide resolved

CraftSpider added 3 commits December 5, 2024 19:59

Most review comments - still missing shim tests

9a76a85

Don't leak miri implementation details

cc6eae5

Fix clippy

3653169

Test file creation and metadata shims directly

a64dbd1

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Dec 6, 2024

RalfJung reviewed Dec 7, 2024

View reviewed changes

tests/pass/shims/windows-fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 7, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 7, 2024

RalfJung reviewed Dec 7, 2024

View reviewed changes

tests/pass/shims/windows-fs.rs Outdated Show resolved Hide resolved

CraftSpider added 7 commits December 7, 2024 18:28

Move windows-fs to pass-dep and use windows_sys

274d90f

Move FdNum to shims/files.rs, use it in FdTable definitions

51273f5

Slightly improve flag handling - parse and validate in one place

aa6bbf6

Fixup imports, compile

938430f

Make metadata handle store the metadata, instead of just a path. Add …

3c2ed8a

…test for this case.

Improve extract_windows_epoch impl

ebfc768

Improve extract_windows_epoch impl comments

d989984

RalfJung reviewed Dec 8, 2024

View reviewed changes

src/shims/windows/fs.rs Outdated Show resolved Hide resolved

CraftSpider added 4 commits December 8, 2024 15:07

Add invalid handle encoding test

e5ada76

Add tests for CREATE_ALWAYS and OPEN_ALWAYS error behavior. Add comme…

52c1676

…nts to code about various quirks

Extract Windows epoch helpers from GetSystemTimeAsFileTime and use th…

5ac99da

…em in file APIs

Merge FileHandle implementation between Unix and Windows

ef5ab7f

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Dec 9, 2024

RalfJung reviewed Dec 12, 2024

View reviewed changes

src/shims/windows/fs.rs Show resolved Hide resolved

RalfJung reviewed Dec 12, 2024

View reviewed changes

src/shims/windows/handle.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 12, 2024

CraftSpider added 3 commits December 12, 2024 22:45

Use u32::MAX constant

6ee99aa

Some fs improvements

a61daf2

Use FdNum more places

92f41ce

RalfJung reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support getting file metadata on Windows #4067

Support getting file metadata on Windows #4067

CraftSpider commented Dec 2, 2024 •

edited

Loading

RalfJung Dec 2, 2024

CraftSpider Dec 6, 2024

RalfJung left a comment

CraftSpider commented Dec 6, 2024 •

edited

Loading

RalfJung commented Dec 6, 2024

CraftSpider commented Dec 6, 2024

RalfJung Dec 7, 2024

RalfJung commented Dec 7, 2024

CraftSpider commented Dec 9, 2024

bors commented Dec 12, 2024

RalfJung Dec 12, 2024

RalfJung Dec 12, 2024

CraftSpider Dec 13, 2024

RalfJung Dec 13, 2024

RalfJung Dec 12, 2024

RalfJung Dec 12, 2024

RalfJung Dec 12, 2024

RalfJung commented Dec 12, 2024

RalfJung Dec 16, 2024

RalfJung Dec 16, 2024

RalfJung Dec 16, 2024

rustbot commented Dec 29, 2024

	OpenExisting => (), // Nothing
	OpenExisting => {} // Default options

Support getting file metadata on Windows #4067

Are you sure you want to change the base?

Support getting file metadata on Windows #4067

Conversation

CraftSpider commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

CraftSpider commented Dec 6, 2024 • edited Loading

RalfJung commented Dec 6, 2024

CraftSpider commented Dec 6, 2024

Choose a reason for hiding this comment

RalfJung commented Dec 7, 2024

CraftSpider commented Dec 9, 2024

bors commented Dec 12, 2024

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

Choose a reason for hiding this comment

RalfJung commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Dec 29, 2024

CraftSpider commented Dec 2, 2024 •

edited

Loading

CraftSpider commented Dec 6, 2024 •

edited

Loading