Page MenuHomePhabricator

[libcxx] Test accessing a directory on windows that gives "access denied" errors
ClosedPublic

Authored by mstorsjo on Mar 8 2021, 2:12 AM.

Details

Summary

Fix handling of skip_permission_denied on windows; after converting
the return value of GetLastError() to a standard error_code, ec.value()
is in the standard errc range, not a native windows error code. This
was missed in 156180727d6c347eda3ba749730707acb8a48093.

The directory "C:\System Volume Information" does seem to exist and
have these properties, but try to verify its existence first.

I guess one could/should factor out the initial setup code to a helper
in support/filesystem_test_helper.h. @curdeius, WDYT?

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Mar 8 2021, 2:12 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 2:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius requested changes to this revision.Mar 8 2021, 4:43 AM

We certainly need to factor it out into some helper in support/filesystem_test_helper.h as you suggested.
Also, I'm not really at ease with:

if (!found)
  return;

I think that we should check it as a precondition of a test. Just returning if the dir is not found may lead to some stupid errors and dead tests.
Is there any case where this path would be inaccessible? MinGW? I mean apart from having a very old OS or no C: partition?

This revision now requires changes to proceed.Mar 8 2021, 4:43 AM
curdeius added inline comments.Mar 8 2021, 4:48 AM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
88

FYI. If you really need a file instead of a directory, there are system files in C:\ (usually, but they can be moved): hiberfil.sys, pagefile.sys, swapfile.sys.

We certainly need to factor it out into some helper in support/filesystem_test_helper.h as you suggested.
Also, I'm not really at ease with:

if (!found)
  return;

I think that we should check it as a precondition of a test. Just returning if the dir is not found may lead to some stupid errors and dead tests.
Is there any case where this path would be inaccessible? MinGW? I mean apart from having a very old OS or no C: partition?

I guess there could be odd installation setups where it isn't available, but as you say, it seems to be available on most setups, so it might be best to just require that it is, and clearly safer against silently missing testcases. And if someone actually runs these tests on windows and fails due to this, they can of course speak up.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
88

Yeah. For this case it doesn't matter if it's a file or directory, but perhaps it's clearer to rename file to path here.

mstorsjo updated this revision to Diff 328980.Mar 8 2021, 5:14 AM

Moved the setup code to a helper function, strictly requiring that
the path exists for the tests to pass.

Not using TEST_*() macros in the helper header, as the individual test
cases are written in a variety of ways, and those macros only work in
certain setups. (Alternatively one could forcibly do assert() in the
helper header.)

curdeius requested changes to this revision.Mar 8 2021, 8:19 AM

Almost there. :)

libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.cons/path.pass.cpp
152

Nit: full stops at end of comments. And it seems that this comment is just truncated.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp
76

For the ease of reading, please swap the branches (here and in other tests):

#ifdef _WIN32
// current else
#else
// current if
#endif
88

Yep, that's good!

libcxx/test/support/filesystem_test_helper.h
674

Typo: Inaccessible (double 's').

674

Nit: why not snake_case? The same for endIt.
Or actually. Never mind, we have GetTestEC too.

This revision now requires changes to proceed.Mar 8 2021, 8:19 AM
mstorsjo updated this revision to Diff 329036.Mar 8 2021, 8:40 AM

Fixed a typo in the function name, flipped the order of ifdefs, fixed
a truncated comment, and expanded these comments a little bit.

curdeius accepted this revision as: curdeius.Mar 8 2021, 8:50 AM

LGTM. Thanks for the fast update!

I think that we should check it as a precondition of a test. Just returning if the dir is not found may lead to some stupid errors and dead tests.
Is there any case where this path would be inaccessible? MinGW? I mean apart from having a very old OS or no C: partition?

I guess there could be odd installation setups where it isn't available, but as you say, it seems to be available on most setups, so it might be best to just require that it is, and clearly safer against silently missing testcases. And if someone actually runs these tests on windows and fails due to this, they can of course speak up.

Ok, speaking up myself here: If running tests within wine instead of on an actual windows installation, this path doesn't exist.

Running tests in wine might not be a situation that we want to explicitly support, but it does overall work fairly well. (The filesystem tests almost all work flawlessly there except for some symlink related corner cases.)

Ok, speaking up myself here: If running tests within wine instead of on an actual windows installation, this path doesn't exist.

Running tests in wine might not be a situation that we want to explicitly support, but it does overall work fairly well. (The filesystem tests almost all work flawlessly there except for some symlink related corner cases.)

Then again, it's not much of an issue to carry a local patch that allows this failure, for branches where I test with wine (I have a commit that adds a other executor, like run.py and ssh.py, for wrapping the tools with wine, that probably isn't to be upstreamed anyway).

Ping for second approval

Mordante accepted this revision.Mar 11 2021, 10:25 AM
Mordante added a subscriber: Mordante.

LGTM!

This revision is now accepted and ready to land.Mar 11 2021, 10:25 AM

@curdeius I found another exception to this issue, which seems rather relevant actually...

If the test process is launched from within bash on windows, then suddenly "C:\System Volume Information" is accessible. (I have no idea why though. All versions of bash on windows come with some sort of posix emulation layer - cygwin/msys or derivatives thereof - and I guess that the emulation layer does something funky...) It doesn't matter if one launches a regular cmd.exe shell again within bash, and if one tries to restore the set of env vars to what it is normally, that dir still is fully accessible in such processes.

I hadn't run into the issue when I'm doing cross testing (where the executables get launched via ssh into a WSL subsystem, sidestepping the whole bash issue).

Currently, bash is required for running tests - I'm looking into getting rid of that requirement, but even if they are runnable without bash, users may end up running things in bash (and even if bash is optional, the testsuite would kinda be set up to prefer using bash if available).

With that in mind, do you think it's reasonable to verify that the "C:\System Volume Information" that we found has got the behaviour that we expect (i.e. signals errors already at exists(path, ec)), and if it doesn't behaves as expected, we'd let those tests pass (possibly with a warning printed)?

curdeius added a comment.EditedMar 18 2021, 4:10 AM

How do you call bash then? Which bash is this?
I get ls: cannot open directory '[/mnt]/c/System Volume Information/': Permission denied from both WSL bash (C:\Windows\System32\bash.exe) and git bash d:\Program Files\Git\bin\bash.exe.
Wouldn't you run it as admin?
BTW, I can't access it even when running bash as Administrator nor using sudo inside.

How do you call bash then? Which bash is this?
I get ls: cannot open directory '[/mnt]/c/System Volume Information/': Permission denied from both WSL bash (C:\Windows\System32\bash.exe) and git bash d:\Program Files\Git\bin\bash.exe.
Wouldn't you run it as admin?
BTW, I can't access it even when running bash as Administrator nor using sudo inside.

I'm testing it with git bash (and with an msys2 shell), on a setup with Windows Server 2019, running as Administrator user. In a regular cmd shell I can't access the directory.

I tested on a normal desktop installation now, and there I see the same as you, that it works as we'd expect and we can't access the directory in any setup. So apparently there's something different with the server versions or the installations of server I'm using (plain new Windows Server instance on Amazon EC2 with only a few tools installed).

Oh, and also, the windows buildkite CI runner runs tests within Windows Containers (controlled via Docker), as far as I understand. And within such a container, that whole directory isn't available at all either...