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

[mstorsjo]

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?

[curdeius]

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?

[curdeius]

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.

[mstorsjo]

In D98166#2610860, @curdeius wrote:

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]

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]

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.

[mstorsjo]

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

[curdeius]

LGTM. Thanks for the fast update!

[mstorsjo]

In D98166#2610897, @mstorsjo wrote:

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.)

[mstorsjo]

In D98166#2613267, @mstorsjo wrote:

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).

[mstorsjo]

Ping for second approval

[Mordante]

LGTM!

[mstorsjo]

@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]

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.

[mstorsjo]

In D98166#2634181, @curdeius wrote:

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).

[mstorsjo]

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...

libcxx/src/filesystem/directory_iterator.cpp

if (root.native().empty()) {
       ec = make_error_code(errc::no_such_file_or_directory);
       return;
     }
     __stream_ = ::FindFirstFileW((root / "*").c_str(), &__data_);
     if (__stream_ == INVALID_HANDLE_VALUE) {
       ec = detail::make_windows_error(GetLastError());
       const bool ignore_permission_denied =
           bool(opts & directory_options::skip_permission_denied);
-      if (ignore_permission_denied && ec.value() == ERROR_ACCESS_DENIED)
+      if (ignore_permission_denied &&
+          ec.value() == static_cast<int>(errc::permission_denied))
         ec.clear();
       return;
     }
     if (!assign())
       advance(ec);
   }
 
   ~__dir_stream() noexcept {

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

// DNE does not cause the constructor to throw
 
     directory_entry ent_two(static_env.BadSymlink);
     TEST_CHECK(ent_two.path() == static_env.BadSymlink);
   }
 }
 
 TEST_CASE(path_ctor_cannot_resolve) {
   using namespace fs;
+#ifdef _WIN32
+  // Windows doesn't support setting perms::none to trigger failures
+  // reading directories; test using a special inaccessible directory
+  // instead.
+  const path dir = GetWindowsInaccessibleDir();
+  TEST_REQUIRE(!dir.empty());
+  const path file = dir / "file";
+  {
+    std::error_code ec = GetTestEC();
+    directory_entry ent(file, ec);
+    TEST_CHECK(ErrorIs(ec, std::errc::no_such_file_or_directory));
+    TEST_CHECK(ent.path() == file);
+  }
+  {
+    TEST_CHECK_NO_THROW(directory_entry(file));
+  }
+#else
   scoped_test_env env;
   const path dir = env.create_dir("dir");
   const path file = env.create_file("dir/file", 42);
   const path file_out_of_dir = env.create_file("file1", 101);
   const path sym_out_of_dir = env.create_symlink("dir/file", "sym");
   const path sym_in_dir = env.create_symlink("dir/file1", "dir/sym2");
   permissions(dir, perms::none);
 
permissions(dir, perms::none);
     TEST_CHECK(!ec);
     TEST_CHECK(ent.path() == sym_out_of_dir);
   }
   {
     TEST_CHECK_NO_THROW(directory_entry(file));
     TEST_CHECK_NO_THROW(directory_entry(sym_in_dir));
     TEST_CHECK_NO_THROW(directory_entry(sym_out_of_dir));
   }
+#endif
 }
 
 TEST_SUITE_END()

libcxx/test/std/input.output/filesystems/class.directory_iterator/directory_iterator.members/ctor.pass.cpp

for (path const& testPath : testPaths)
             TEST_CHECK_THROW(filesystem_error, directory_iterator(testPath, opts));
         }
     }
 }
 
 TEST_CASE(access_denied_test_case)
 {
     using namespace fs;
+#ifdef _WIN32
+    // Windows doesn't support setting perms::none to trigger failures
+    // reading directories; test using a special inaccessible directory
+    // instead.
+    const path testDir = GetWindowsInaccessibleDir();
+    TEST_REQUIRE(!testDir.empty());
+#else
     scoped_test_env env;
     path const testDir = env.make_env_path("dir1");
     path const testFile = testDir / "testFile";
     env.create_dir(testDir);
     env.create_file(testFile, 42);
 
     // Test that we can iterator over the directory before changing the perms
     {
         directory_iterator it(testDir);
         TEST_REQUIRE(it != directory_iterator{});
     }
     // Change the permissions so we can no longer iterate
     permissions(testDir, perms::none);
+#endif
 
     // Check that the construction fails when skip_permissions_denied is
     // not given.
     {
         std::error_code ec;
         directory_iterator it(testDir, ec);
         TEST_REQUIRE(ec);
         TEST_CHECK(it == directory_iterator{});

libcxx/test/std/input.output/filesystems/class.rec.dir.itr/rec.dir.itr.members/ctor.pass.cpp

for (path const& testPath : testPaths)
             TEST_CHECK_THROW(filesystem_error, RDI(testPath, opts));
         }
     }
 }
 
 TEST_CASE(access_denied_test_case)
 {
     using namespace fs;
+#ifdef _WIN32
+    // Windows doesn't support setting perms::none to trigger failures
+    // reading directories; test using a special inaccessible directory
+    // instead.
+    const path testDir = GetWindowsInaccessibleDir();
+    TEST_REQUIRE(!testDir.empty());
+#else
     scoped_test_env env;
     path const testDir = env.make_env_path("dir1");
     path const testFile = testDir / "testFile";
     env.create_dir(testDir);
     env.create_file(testFile, 42);
 
     // Test that we can iterator over the directory before changing the perms
     {
         RDI it(testDir);
         TEST_REQUIRE(it != RDI{});
     }
 
     // Change the permissions so we can no longer iterate
     permissions(testDir, perms::none);
+#endif
 
     // Check that the construction fails when skip_permissions_denied is
     // not given.
     {
         std::error_code ec;
         RDI it(testDir, ec);
         TEST_REQUIRE(ec);
         TEST_CHECK(it == RDI{});

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

TEST_CASE(test_exist_not_found)
 
     std::error_code ec = GetTestEC();
     TEST_CHECK(exists(p, ec) == false);
     TEST_CHECK(!ec);
 }
 
 TEST_CASE(test_exists_fails)
 {
+#ifdef _WIN32
+    // Windows doesn't support setting perms::none to trigger failures
+    // reading directories; test using a special inaccessible directory
+    // instead.
+    const path p = GetWindowsInaccessibleDir();
+    TEST_REQUIRE(!p.empty());
+#else
     scoped_test_env env;
     const path dir = env.create_dir("dir");
-    const path file = env.create_file("dir/file", 42);
+    const path p = env.create_file("dir/file", 42);
     permissions(dir, perms::none);
+#endif
 
     std::error_code ec;
-    TEST_CHECK(exists(file, ec) == false);
+    TEST_CHECK(exists(p, ec) == false);
     TEST_CHECK(ec);
 
-    TEST_CHECK_THROW(filesystem_error, exists(file));
+    TEST_CHECK_THROW(filesystem_error, exists(p));
 }
 
 #ifndef _WIN32
 // Checking for the existence of an invalid long path name doesn't
 // trigger errors on windows.
 TEST_CASE(test_name_too_long) {
     std::string long_name(2500, 'a');
     const path file(long_name);
 
     std::error_code ec;
     TEST_CHECK(exists(file, ec) == false);
     TEST_CHECK(ec);
 }
 #endif
 
 TEST_SUITE_END()

libcxx/test/support/filesystem_test_helper.h

void check_libcxx_string(fs::filesystem_error const& Err) {
     }
   }
 
   ExceptionChecker(ExceptionChecker const&) = delete;
   ExceptionChecker& operator=(ExceptionChecker const&) = delete;
 
 };
 
+inline fs::path GetWindowsInaccessibleDir() {
+  // Only makes sense on windows, but the code can be compiled for
+  // any platform.
+  const fs::path dir("C:\\System Volume Information");
+  std::error_code ec;
+  const fs::path root("C:\\");
+  fs::directory_iterator it(root, ec);
+  if (ec)
+    return fs::path();
+  const fs::directory_iterator endIt{};
+  while (it != endIt) {
+    const fs::directory_entry &ent = *it;
+    if (ent == dir) {
+      // Basic sanity checks on the directory_entry
+      if (!ent.exists())
+        return fs::path();
+      if (!ent.is_directory())
+        return fs::path();
+      return ent;
+    }
+    ++it;
+  }
+  return fs::path();
+}
+
 #endif /* FILESYSTEM_TEST_HELPER_HPP */