[filesystem] Don't error on weird system files

[strega-nil-ms]

This changes from using GetFileAttributesExW to FindFirstFileW,
since FindFirstFileW gets the information from the directory
and thus doesn't error out.

Fixes #2370

This will require serious testing to make sure the behavior is the same.

Related issue in Rust: rust-lang/rust#96980

[StephanTLavavej]

Looks good, just one more round of comments and I think this will be ready to go!

[fsb4000]

I didn't test anything but I just had read a comment from the Rust issue:

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.

rust-lang/rust#96980 (comment)

so I decided to copy the comment for those who didn't follow the link

[ChrisDenton]

While I think the two user scenario is more realistic, you can get the same outcome by simply denying yourself list folder permissions on the file's parent directory. This is simpler to test.

[StephanTLavavej]

@strega-nil-ms I pushed a conflict-free merge with main (as filesystem.cpp had changed), followed by minor changes for @miscco's feedback (detaching comments) and mine (limiting variable scope), followed by a correctness fix. I then verified that your branch fixes the originally reported bug:

hiberfil.sys size

This remained unchanged throughout testing:

C:\Temp>dir /ah C:\hiberfil.sys
[...]
05/11/2022  03:32 AM    13,648,355,328 hiberfil.sys

Test code

C:\Temp>type meow.cpp

#include <filesystem>
#include <iostream>
using namespace std;
using namespace std::filesystem;

int main() {
    try {
        const auto bytes = file_size(LR"(C:\hiberfil.sys)");
        cout << "hiberfil.sys size: " << bytes << endl;
    } catch (const filesystem_error& e) {
        cout << "filesystem_error: " << e.what() << endl;
    }
}

Current behavior of main

C:\Temp>cl /EHsc /nologo /W4 /std:c++17 meow.cpp && meow
meow.cpp
filesystem_error: file_size: The process cannot access the file because it is being used by another process.: "C:\hiberfil.sys"

Before final correctness fix

C:\Temp>cl /EHsc /nologo /W4 /std:c++17 meow.cpp && meow
meow.cpp
hiberfil.sys size: 12884901891

• 13648355328 is 0x3'2D81'6000
• 12884901891 is 0x3'0000'0003

After final correctness fix

C:\Temp>cl /EHsc /nologo /W4 /std:c++17 meow.cpp && meow
meow.cpp
hiberfil.sys size: 13648355328

With this manual testing, and your new "fallback" approach that's activated only when the original codepath fails, I think this is ready to ship. (Entirely replacing the original codepath would be much riskier and require more extensive testing.)

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for investigating and fixing this bug affecting hiberfil.sys and more!

🐞 🔍 🐻