[VFS] Fix vfsoverlay assertion due to RedirectingFileSystem path handling.

[john.brawn]

Summary

The ClangScanDeps/vfsoverlay test can fail on Windows due to it ending up looking up paths that look like c:/foo\..\bar which hits an inconsistency in path handling: RedirectingFileSystemParser sees the c: and decides this is a windows path, canonicalize sees the first / and decides this is a posix path, and path::begin uses the native path style. The end result of this is we end up seeing '..' as a traversal component and hit an assertion failure.

Fix this by making lookupPath check the absolute path style, the same as RedirectingFileSystemParser does, and pass this through to path::begin.

[amccarth]

I'll look at this more later today, but here are my initial thoughts.

ClangScanDeps/vfsoverlay test can fail on Windows

"can fail"? Are you saying the results are flaky? I haven't seen this test fail.

I never saw a situation where an external path could start with C:/ rather than C:\. The keys for VFS are sometimes weird hybrids of Posix and Windows styles, but I can't figure out how a VFS lookup would result in an absolute path with that kind of prefix.

llvm/lib/Support/VirtualFileSystem.cpp

1002

I was hesitant to do it this way, but, for an arbitrary path, I didn't see a better alternative. Maybe there's something smarter we can do here so that you don't have to special case the fix.

1694

Nit: With the change below, this comment becomes slightly misleading. remove_dots doesn't remove a leading "./". Of course, there won't be one because the path is absolute.

1697

I'm uneasy with this. The intent of introducing canonicalize was to reduce the inconsistencies in several parts of this code: some considered path style, others didn't; some removed certain dot patterns but not others.

I'd prefer it if we could fix canonicalize rather than to try to work around a special case at one call site.

1701

At one point, I, too, put the path_style here, but it has no tangible difference, so I thought it was best to leave it out.

llvm/unittests/Support/VirtualFileSystemTest.cpp

2237

This doesn't seem like a valid test. It's one thing for some simplistic path code to append Windows and Posix path fragments together, resulting in a Windows-like path that's syntactically incorrect. But it's another thing to feed it syntactically incorrect absolute paths. That will always eventually lead to ambiguous situations where there is no correct option for the code.

I think the VFS YAML reader should reject yaml. Specifically an absolute external-contents path with incorrect syntax.

[And, yes, it _is_ incorrect syntax on Windows. Most WinAPIs will correct the slash direction during normalization, but Windows has local device paths (\\?\) that bypass normalization and UNC paths which require the correct separators.]

[john.brawn]

In D74488#1872608, @amccarth wrote:

I'll look at this more later today, but here are my initial thoughts.

ClangScanDeps/vfsoverlay test can fail on Windows

"can fail"? Are you saying the results are flaky? I haven't seen this test fail.

I never saw a situation where an external path could start with C:/ rather than C:\. The keys for VFS are sometimes weird hybrids of Posix and Windows styles, but I can't figure out how a VFS lookup would result in an absolute path with that kind of prefix.

I think it depends on how exactly the test is run. I'm seeing this in our continuous integration setup which I don't fully understand, but the relevant part of the test log is

: 'RUN: at line 1'; rm -rf C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir
: 'RUN: at line 2'; rm -rf C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb
: 'RUN: at line 3'; mkdir -p C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir
: 'RUN: at line 4'; cp C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\src\llvm-project\clang\test\ClangScanDeps\vfsoverlay.cpp C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/vfsoverlay_input.cpp
: 'RUN: at line 5'; sed -e "s|DIR|C:/work/jenkins_slave/workspace/armcompiler/0.0/armclang-windows/build-asserts-true/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g" C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\src\llvm-project\clang\test\ClangScanDeps/Inputs/vfsoverlay.yaml > C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/vfsoverlay.yaml
: 'RUN: at line 6'; mkdir C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/Inputs
: 'RUN: at line 7'; cp C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\src\llvm-project\clang\test\ClangScanDeps/Inputs/header.h C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/Inputs/header.h
: 'RUN: at line 8'; sed -e "s|DIR|C:/work/jenkins_slave/workspace/armcompiler/0.0/armclang-windows/build-asserts-true/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g" C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\src\llvm-project\clang\test\ClangScanDeps/Inputs/vfsoverlay_cdb.json > C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb
: 'RUN: at line 10'; clang-scan-deps -compilation-database C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb -j 1 | c:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\build-asserts-true\bin\filecheck.exe C:\work\jenkins_slave\workspace\armcompiler\0.0\armclang-windows\src\llvm-project\clang\test\ClangScanDeps\vfsoverlay.cpp

Those sed lines with forward slashes are coming from lit expanding "%/t.dir" in vfsoverlay.cpp.

[amccarth]

Thanks for the extra info!

On my machine I'm seeing the same issue with the sed lines generating bad path syntax, yet the test still passes! (And, yes, assertions are enabled.)

I'd like to be able to repro the problem before blessing any particular fix.

Is it possible your build bot isn't consistently synced?

D:\src\llvm\build\ninja_dbg>python bin/llvm-lit.py -v -vv -a tools/clang/test/ClangScanDeps/vfsoverlay.cpp
llvm-lit.py: D:/src/llvm/llvm-project/llvm\utils\lit\lit\llvm\config.py:342: note: using clang: d:\src\llvm\build\ninja_dbg\bin\clang.exe
-- Testing: 1 tests, 1 workers --
PASS: Clang :: ClangScanDeps/vfsoverlay.cpp (1 of 1)
Script:
--
: 'RUN: at line 1';   rm -rf D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir
: 'RUN: at line 2';   rm -rf D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb
: 'RUN: at line 3';   mkdir -p D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir
: 'RUN: at line 4';   cp D:\src\llvm\llvm-project\clang\test\ClangScanDeps\vfsoverlay.cpp D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/vfsoverlay_input.cpp
: 'RUN: at line 5';   sed -e "s|DIR|D:/src/llvm/build/ninja_dbg/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g" D:\src\llvm\llvm-project\clang\test\ClangScanDeps/Inputs/vfsoverlay.yaml > D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/vfsoverlay.yaml
: 'RUN: at line 6';   mkdir D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/Inputs
: 'RUN: at line 7';   cp D:\src\llvm\llvm-project\clang\test\ClangScanDeps/Inputs/header.h D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.dir/Inputs/header.h
: 'RUN: at line 8';   sed -e "s|DIR|D:/src/llvm/build/ninja_dbg/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g" D:\src\llvm\llvm-project\clang\test\ClangScanDeps/Inputs/vfsoverlay_cdb.json > D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb
: 'RUN: at line 10';   clang-scan-deps -compilation-database D:\src\llvm\build\ninja_dbg\tools\clang\test\ClangScanDeps\Output\vfsoverlay.cpp.tmp.cdb -j 1 |    d:\src\llvm\build\ninja_dbg\bin\filecheck.exe D:\src\llvm\llvm-project\clang\test\ClangScanDeps\vfsoverlay.cpp
--
Exit Code: 0

[john.brawn]

I've attached a standalone example:

example.zip1 KBDownload

This should be extracted to c:\work\example, and I see it hitting an assertion using clang-scan-deps built from commit 81cebfd0080e3873d0cef5ee5215b8c97332ff96 using the command-line "clang-scan-deps.exe -compilation-database example.cdb"

I've done some debugging and the chain of events that leads to the assertion being hit is:

• getDirectoryFromFile (clang/lib/Basic/Filemanager.cpp) called with Filename=vfsoverlay_input.cpp
• That calls FileManager::getDirectory with DirName="."
• That calls FileManager::getDirectoryRef, which calls FileManager::getStatValue, which calls FileManager::FixupRelativePath
• FileManager::FixupRelativePath uses path::append to append "." onto the working directory and gets "C:/work/example\." because path::append uses the preferred native separator which is "\"
• Further down the call chain we end up in RedirectingFileSystem::lookupPath with a path of "C:/work/example\.", and the "\." isn't removed because canonicalize thinks "/" is the directory separator.
• path::begin uses the native path style, which on windows means both "/" and "\" are treated as directory separators, so we get the path components ["C:", "/", "work", "example", "."] and on the "." we hit the assertion failure.

[amccarth]

Thanks for the additional info.

I understand why canonicalize would fail to remove the dot in that case. I think the fix should be in canonicalize rather than bypassing it. I have a couple ideas for that.

But first I'm using your example to try to understand how we get in that case on some machines but not others (like mine). The test for any fix would be specious.

I hope to figure this out and propose a different fix today.

[amccarth]

FileManager::FixupRelativePath uses path::append to append "." onto the working directory and gets "C:/work/example\." because path::append uses the preferred native separator which is "\"

This is where we diverge. I'm getting a Windows-style working directory (C:\work\example), so appending the dot yields C:\work\example\., and canonicalize handles that properly.

The working directory is initialized from the external filesystem. On my machine, this is naturally in native host form. In your case, something must be changing it to the (forward) slash form.

Still investigating.

[amccarth]

Ah, with clang-scan-deps the cdb file is overriding the working directory with one in the wrong format.

I don't see why the ClangScanDeps tests don't fail for me, but this is a good lead. I think I'll have this pinned down soon.

[amccarth]

I'm stumped why the regular tests don't repro this for me. Somehow, getDirectoryFromFile is only ever called with a fully resolved path, so it never falls back to setting DirName to .. I'm going to stop chasing this. I can make it happen with your zipped example.

I would prefer to solve the problem by making canonicalize smarter rather than bypassing it. I have a few ideas:

1. Instead of just checking the first slash type, it would first check for a drive letter, which obviously means it's Windows style. If there's no drive letter, fall back to slash check. In the problematic case, it's always an absolute path, so this should work.

2. Have canonicalize resolve dots and double dots itself rather than delegating it to the sys::path::remove_dots and sys::path::remove_leading_dots. This could be done in a slash agnostic (and preserving) way rather than trying to guess the style. The problem boils down to the fact that we have paths in a hybrid style that sys::path was never intended to handle.

3. Fix the test so that it doesn't convert the backslashes to slashes and make the cdb and yaml parsing reject hybrid-style paths. That's probably the most principled thing, but it's also the most fragile and it's not consistent with Clang's general hands-off policy when it comes to manipulating paths.

I'm leaning toward 2.

[amccarth]

I got approach #2 working Friday night. See: https://reviews.llvm.org/D74777

I feel this approach is more general and may head off other problems we haven't yet spotted.

If you are OK with it, perhaps you could splice my patch into here with your tests (or vice versa). I could do it as well, but I may not get to it for a few days, so feel free to combine them yourself if you want to land it sooner.

[john.brawn]

Abandoning this old patch that I haven't looked at in a long time.

llvm/lib/Support/VirtualFileSystem.cpp

 
 /// Removes leading "./" as well as path components like ".." and ".".
 static llvm::SmallString<256> canonicalize(llvm::StringRef Path) {
   // First detect the path style in use by checking the first separator.
   llvm::sys::path::Style style = llvm::sys::path::Style::native;
   const size_t n = Path.find_first_of("/\\");
   if (n != static_cast<size_t>(-1))
     style = (Path[n] == '/') ? llvm::sys::path::Style::posix
                              : llvm::sys::path::Style::windows;
 
   // Now remove the dots.  Explicitly specifying the path style prevents the
   // direction of the slashes from changing.
   llvm::SmallString<256> result =
       llvm::sys::path::remove_leading_dotslash(Path, style);
   llvm::sys::path::remove_dots(result, /*remove_dot_dot=*/true, style);
   return result;
 }

 RedirectingFileSystem::lookupPath(const Twine &Path_) const {
   SmallString<256> Path;
   Path_.toVector(Path);
 
   // Handle relative paths
   if (std::error_code EC = makeAbsolute(Path))
     return EC;
 
+  // Now that we have an absolute path we can examine it to determine the
+  // path style.
+  sys::path::Style path_style = sys::path::Style::native;
+  if (sys::path::is_absolute(Path, sys::path::Style::posix)) {
+    path_style = sys::path::Style::posix;
+  } else if (sys::path::is_absolute(Path, sys::path::Style::windows)) {
+    path_style = sys::path::Style::windows;
+  }
+
   // Canonicalize path by removing ".", "..", "./", components. This is
   // a VFS request, do not bother about symlinks in the path components
   // but canonicalize in order to perform the correct entry search.
-  Path = canonicalize(Path);
+  llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true, path_style);
   if (Path.empty())
     return make_error_code(llvm::errc::invalid_argument);
 
-  sys::path::const_iterator Start = sys::path::begin(Path);
+  sys::path::const_iterator Start = sys::path::begin(Path, path_style);
   sys::path::const_iterator End = sys::path::end(Path);
   for (const auto &Root : Roots) {
     ErrorOr<RedirectingFileSystem::Entry *> Result =
         lookupPath(Start, End, Root.get());
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
 
   llvm::ErrorOr<vfs::Status> Status = FS->status("bar/a");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
 
   Status = FS->status("foo/a");
   ASSERT_TRUE(Status.getError());
 }
+
+TEST_F(VFSFromYAMLTest, BackslashWindowsPath) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  Lower->addRegularFile("c:\\root\\foo\\bar\\a");
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'roots': [\n"
+      "{\n"
+      "  'type': 'directory',\n"
+      "  'name': 'c:\\root\\',\n"
+      "  'contents': [ {\n"
+      "                  'type': 'file',\n"
+      "                  'name': 'file1',\n"
+      "                  'external-contents': 'c:\\root\\foo\\bar\\a'\n"
+      "                }\n"
+      "              ]\n"
+      "}\n"
+      "]\n"
+      "}",
+      Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr<vfs::OverlayFileSystem> O(
+      new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  ErrorOr<vfs::Status> S = O->status("c:\\root\\foo\\..\\file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:\\root\\foo\\bar\\a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:/root/foo/../file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:\\root\\foo\\bar\\a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:/root/foo\\..\\file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:\\root\\foo\\bar\\a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:\\root\\foo/../file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:\\root\\foo\\bar\\a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+}
+
+TEST_F(VFSFromYAMLTest, ForwardslashWindowsPath) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  Lower->addRegularFile("c:/root/foo/bar/a");
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'roots': [\n"
+      "{\n"
+      "  'type': 'directory',\n"
+      "  'name': 'c:/root/',\n"
+      "  'contents': [ {\n"
+      "                  'type': 'file',\n"
+      "                  'name': 'file1',\n"
+      "                  'external-contents': 'c:/root/foo/bar/a'\n"
+      "                }\n"
+      "              ]\n"
+      "}\n"
+      "]\n"
+      "}",
+      Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr<vfs::OverlayFileSystem> O(
+      new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  ErrorOr<vfs::Status> S = O->status("c:\\root\\foo\\..\\file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:/root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:/root/foo/../file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:/root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:/root/foo\\..\\file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:/root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  S = O->status("c:\\root\\foo/../file1");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("c:/root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+}