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

[fs] Path::new(r#"C:\hiberfil.sys"#).exists() returns false, despite the file existing #96980

Closed
strega-nil-ms opened this issue May 12, 2022 · 8 comments · Fixed by #98916
Closed
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@strega-nil-ms
Copy link

strega-nil-ms commented May 12, 2022

I noticed this bug while researching the same bug in https://github.com/microsoft/STL.

// test.rs
use std::{env, fs};
use std::path::Path;

fn main() {
  for p in env::args().skip(1) {
    println!("{:?}.exists() = {}", p, Path::new(p).exists());
    println!("  => (metadata = {})", if fs::metadata(p).is_err() { "error" } else { "ok" });
  }
}

Given test.exe C:\hiberfil.sys, I expected:

"C:\\hiberfil.sys".exists() = true
  => (metadata = ok)

Instead, this happened:

"C:\\hiberfil.sys".exists() = false
  => (metadata = error)

Solution

The related issue in the STL is microsoft/STL#2370; the PR to fix this in the STL is microsoft/STL#2715.

The problem is that fs::metadata opens the file; this is not allowed for the following four files (not sure if there are additional files which act similarly):

  • C:\DumpStack.log.tmp
  • C:\hiberfil.sys
  • C:\pagefile.sys
  • C:\swapfile.sys

We can get around this by using FindFirstFileW instead of open(); is that something that's desired, or is this considered a weird enough use case that we aren't interested in fixing it?

@strega-nil-ms strega-nil-ms added the C-bug Category: This is a bug. label May 12, 2022
@nagisa
Copy link
Member

nagisa commented May 12, 2022

Documentation explicitly documents that exists is equivalent to accessing metadata of the file, doesn't it? Changing this behaviour would be a breaking change I'm afraid we wouldn't be able to afford.

@strega-nil-ms
Copy link
Author

strega-nil-ms commented May 12, 2022

@nagisa true, the question was whether fs::metadata(Path) could use FindFirstFileW() instead of CreateFileW() followed by GetFileInformationByHandle(); that's how we're solving this in the C++ standard library.

This function currently corresponds to the stat function on Unix and the GetFileAttributesEx function on Windows. Note that, this may change in the future.

(I'll note that this documentation is now wrong, since it now uses GetFileInformationByHandle)

This would mean that the Metadata returned from fs::metadata(p) would now have None for

  • volume_serial_number
  • number_of_links
  • file_index

but otherwise it'd be complete (although for reparse points, we'd still have to open the file).

We could additionally do FindFirstFileW() followed by CreateFileW() -> GetFileInformationByHandle(), to fill out those fields. I'll note that we already have this behavior on UWP, and when getting metadata from a DirEntry.

@ChrisDenton
Copy link
Member

ChrisDenton commented May 12, 2022

Hm, changing fs::metadata is not something I'd be minded to do lightly. At least not without further investigation. Some questions that spring to mind:

  • What happens if the user does not have permission to read the directory but does have permission to read the file?
  • Can we get all the metadata that's needed? Ah your edit answered that.
  • Does this affect performance?

@ChrisDenton ChrisDenton added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 12, 2022
@strega-nil-ms
Copy link
Author

@ChrisDenton given that it's reading the directory, it should make performance better (assuming one doesn't do the double call thing).

I'm personally not that invested, I just thought I'd share with y'all since I have to fix this bug for the STL 😛

@ChrisDenton
Copy link
Member

It's great that we have this report! I just want to make sure we investigate fully before making changes that may affect users of the standard library.

I'll also note we do have the unstable std::fs::try_exists whose behaviour can change.

@ChrisDenton
Copy link
Member

ChrisDenton commented May 12, 2022

What happens if the user does not have permission to read the directory but does have permission to read the file?

Ok I tested with two users. For the sake of this example, I'll call them "Admin" and "User".

As Admin I created a directory and denied User all access. Within the directory I then created a "test.txt" file and granted User access to just that file.

As User, I tried using FindFirstFile to test if the file exists. This failed with ERROR_ACCESS_DENIED. I then tried using GetFileAttributes which succeeded. As does opening the file with CreateFile,

Thus FindFirstFile can fail in situations where directly accessing the file would succeed.

@rgwood
Copy link
Contributor

rgwood commented Jun 2, 2022

FYI, Nushell has run into this in the wild: nushell/nushell#4975

We'll work around it by falling back to FindFirstFileW if fs::metadata fails. Would be nice if fs::metadata could do something similar automatically, but I appreciate that there's a high bar for backwards compatibility and performance here.

@ChrisDenton
Copy link
Member

To be clear, using FindFirstFile as a fallback (rather than replacement) answers my concerns and I think would be the best solution to this issue.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 18, 2022
Windows: Use `FindFirstFileW` for getting the metadata of locked system files

Fixes rust-lang#96980

Usually opening a file handle with access set to metadata only will always succeed, even if the file is locked. However some special system files, such as `C:\hiberfil.sys`, are locked by the system in a way that denies even that. So as a fallback we try reading the cached metadata from the directory.

Note that the test is a bit iffy. I don't know if `hiberfil.sys` actually exists in the CI.

r? rust-lang/libs
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 18, 2022
Windows: Use `FindFirstFileW` for getting the metadata of locked system files

Fixes rust-lang#96980

Usually opening a file handle with access set to metadata only will always succeed, even if the file is locked. However some special system files, such as `C:\hiberfil.sys`, are locked by the system in a way that denies even that. So as a fallback we try reading the cached metadata from the directory.

Note that the test is a bit iffy. I don't know if `hiberfil.sys` actually exists in the CI.

r? rust-lang/libs
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 18, 2022
Windows: Use `FindFirstFileW` for getting the metadata of locked system files

Fixes rust-lang#96980

Usually opening a file handle with access set to metadata only will always succeed, even if the file is locked. However some special system files, such as `C:\hiberfil.sys`, are locked by the system in a way that denies even that. So as a fallback we try reading the cached metadata from the directory.

Note that the test is a bit iffy. I don't know if `hiberfil.sys` actually exists in the CI.

r? rust-lang/libs
@bors bors closed this as completed in 8039567 Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants