From df6c3175af91dd84558b619fa4267550ffb34f1e Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Thu, 14 Mar 2024 13:41:19 -0700 Subject: [PATCH] [llvm][vfs] Preserve paths for fallback/fallthrough in RedirectingFileSystem When we lookup in the external filesystem, do not remove . and .. components from the original path. For .. this is a correctness issue in the presence of symlinks, while for . it is simply better practice to preserve the original path to better match the behaviour of other filesystems. The only modification we need is to apply the working directory, since it could differ from the external filesystem. rdar://123655660 --- llvm/include/llvm/Support/VirtualFileSystem.h | 8 +- llvm/lib/Support/VirtualFileSystem.cpp | 88 +++++++++---------- .../Support/VirtualFileSystemTest.cpp | 65 ++++++++++++++ 3 files changed, 112 insertions(+), 49 deletions(-) diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index ef1fac92c2fa4..770ca8764426a 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -929,12 +929,12 @@ class RedirectingFileSystem /// 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. - std::error_code makeCanonical(SmallVectorImpl &Path) const; + std::error_code makeCanonicalForLookup(SmallVectorImpl &Path) const; /// Get the File status, or error, from the underlying external file system. /// This returns the status with the originally requested name, while looking - /// up the entry using the canonical path. - ErrorOr getExternalStatus(const Twine &CanonicalPath, + /// up the entry using a potentially different path. + ErrorOr getExternalStatus(const Twine &LookupPath, const Twine &OriginalPath) const; /// Make \a Path an absolute path. @@ -1022,7 +1022,7 @@ class RedirectingFileSystem llvm::SmallVectorImpl &Entries) const; /// Get the status for a path with the provided \c LookupResult. - ErrorOr status(const Twine &CanonicalPath, const Twine &OriginalPath, + ErrorOr status(const Twine &LookupPath, const Twine &OriginalPath, const LookupResult &Result); public: diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index 051dd2a67d120..057f8eae0552c 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -1344,7 +1344,7 @@ std::error_code RedirectingFileSystem::isLocal(const Twine &Path_, SmallString<256> Path; Path_.toVector(Path); - if (makeCanonical(Path)) + if (makeAbsolute(Path)) return {}; return ExternalFS->isLocal(Path, Result); @@ -1411,7 +1411,7 @@ directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir, SmallString<256> Path; Dir.toVector(Path); - EC = makeCanonical(Path); + EC = makeAbsolute(Path); if (EC) return {}; @@ -2261,8 +2261,8 @@ void RedirectingFileSystem::LookupResult::getPath( llvm::sys::path::append(Result, E->getName()); } -std::error_code -RedirectingFileSystem::makeCanonical(SmallVectorImpl &Path) const { +std::error_code RedirectingFileSystem::makeCanonicalForLookup( + SmallVectorImpl &Path) const { if (std::error_code EC = makeAbsolute(Path)) return EC; @@ -2277,12 +2277,16 @@ RedirectingFileSystem::makeCanonical(SmallVectorImpl &Path) const { ErrorOr RedirectingFileSystem::lookupPath(StringRef Path) const { + llvm::SmallString<128> CanonicalPath(Path); + if (std::error_code EC = makeCanonicalForLookup(CanonicalPath)) + return EC; + // RedirectOnly means the VFS is always used. if (UsageTrackingActive && Redirection == RedirectKind::RedirectOnly) HasBeenUsed = true; - sys::path::const_iterator Start = sys::path::begin(Path); - sys::path::const_iterator End = sys::path::end(Path); + sys::path::const_iterator Start = sys::path::begin(CanonicalPath); + sys::path::const_iterator End = sys::path::end(CanonicalPath); llvm::SmallVector Entries; for (const auto &Root : Roots) { ErrorOr Result = @@ -2358,14 +2362,14 @@ static Status getRedirectedFileStatus(const Twine &OriginalPath, } ErrorOr RedirectingFileSystem::status( - const Twine &CanonicalPath, const Twine &OriginalPath, + const Twine &LookupPath, const Twine &OriginalPath, const RedirectingFileSystem::LookupResult &Result) { if (std::optional ExtRedirect = Result.getExternalRedirect()) { - SmallString<256> CanonicalRemappedPath((*ExtRedirect).str()); - if (std::error_code EC = makeCanonical(CanonicalRemappedPath)) + SmallString<256> RemappedPath((*ExtRedirect).str()); + if (std::error_code EC = makeAbsolute(RemappedPath)) return EC; - ErrorOr S = ExternalFS->status(CanonicalRemappedPath); + ErrorOr S = ExternalFS->status(RemappedPath); if (!S) return S; S = Status::copyWithNewName(*S, *ExtRedirect); @@ -2375,13 +2379,13 @@ ErrorOr RedirectingFileSystem::status( } auto *DE = cast(Result.E); - return Status::copyWithNewName(DE->getStatus(), CanonicalPath); + return Status::copyWithNewName(DE->getStatus(), LookupPath); } ErrorOr -RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath, +RedirectingFileSystem::getExternalStatus(const Twine &LookupPath, const Twine &OriginalPath) const { - auto Result = ExternalFS->status(CanonicalPath); + auto Result = ExternalFS->status(LookupPath); // The path has been mapped by some nested VFS, don't override it with the // original path. @@ -2391,38 +2395,37 @@ RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath, } ErrorOr RedirectingFileSystem::status(const Twine &OriginalPath) { - SmallString<256> CanonicalPath; - OriginalPath.toVector(CanonicalPath); + SmallString<256> Path; + OriginalPath.toVector(Path); - if (std::error_code EC = makeCanonical(CanonicalPath)) + if (std::error_code EC = makeAbsolute(Path)) return EC; if (Redirection == RedirectKind::Fallback) { // Attempt to find the original file first, only falling back to the // mapped file if that fails. - ErrorOr S = getExternalStatus(CanonicalPath, OriginalPath); + ErrorOr S = getExternalStatus(Path, OriginalPath); if (S) return S; } - ErrorOr Result = - lookupPath(CanonicalPath); + ErrorOr Result = lookupPath(Path); if (!Result) { // Was not able to map file, fallthrough to using the original path if // that was the specified redirection type. if (Redirection == RedirectKind::Fallthrough && isFileNotFound(Result.getError())) - return getExternalStatus(CanonicalPath, OriginalPath); + return getExternalStatus(Path, OriginalPath); return Result.getError(); } - ErrorOr S = status(CanonicalPath, OriginalPath, *Result); + ErrorOr S = status(Path, OriginalPath, *Result); if (!S && Redirection == RedirectKind::Fallthrough && isFileNotFound(S.getError(), Result->E)) { // Mapped the file but it wasn't found in the underlying filesystem, // fallthrough to using the original path if that was the specified // redirection type. - return getExternalStatus(CanonicalPath, OriginalPath); + return getExternalStatus(Path, OriginalPath); } return S; @@ -2471,30 +2474,27 @@ File::getWithPath(ErrorOr> Result, const Twine &P) { ErrorOr> RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) { - SmallString<256> CanonicalPath; - OriginalPath.toVector(CanonicalPath); + SmallString<256> Path; + OriginalPath.toVector(Path); - if (std::error_code EC = makeCanonical(CanonicalPath)) + if (std::error_code EC = makeAbsolute(Path)) return EC; if (Redirection == RedirectKind::Fallback) { // Attempt to find the original file first, only falling back to the // mapped file if that fails. - auto F = File::getWithPath(ExternalFS->openFileForRead(CanonicalPath), - OriginalPath); + auto F = File::getWithPath(ExternalFS->openFileForRead(Path), OriginalPath); if (F) return F; } - ErrorOr Result = - lookupPath(CanonicalPath); + ErrorOr Result = lookupPath(Path); if (!Result) { // Was not able to map file, fallthrough to using the original path if // that was the specified redirection type. if (Redirection == RedirectKind::Fallthrough && isFileNotFound(Result.getError())) - return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath), - OriginalPath); + return File::getWithPath(ExternalFS->openFileForRead(Path), OriginalPath); return Result.getError(); } @@ -2502,22 +2502,21 @@ RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) { return make_error_code(llvm::errc::invalid_argument); StringRef ExtRedirect = *Result->getExternalRedirect(); - SmallString<256> CanonicalRemappedPath(ExtRedirect.str()); - if (std::error_code EC = makeCanonical(CanonicalRemappedPath)) + SmallString<256> RemappedPath(ExtRedirect.str()); + if (std::error_code EC = makeAbsolute(RemappedPath)) return EC; auto *RE = cast(Result->E); - auto ExternalFile = File::getWithPath( - ExternalFS->openFileForRead(CanonicalRemappedPath), ExtRedirect); + auto ExternalFile = + File::getWithPath(ExternalFS->openFileForRead(RemappedPath), ExtRedirect); if (!ExternalFile) { if (Redirection == RedirectKind::Fallthrough && isFileNotFound(ExternalFile.getError(), Result->E)) { // Mapped the file but it wasn't found in the underlying filesystem, // fallthrough to using the original path if that was the specified // redirection type. - return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath), - OriginalPath); + return File::getWithPath(ExternalFS->openFileForRead(Path), OriginalPath); } return ExternalFile; } @@ -2537,28 +2536,27 @@ RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) { std::error_code RedirectingFileSystem::getRealPath(const Twine &OriginalPath, SmallVectorImpl &Output) const { - SmallString<256> CanonicalPath; - OriginalPath.toVector(CanonicalPath); + SmallString<256> Path; + OriginalPath.toVector(Path); - if (std::error_code EC = makeCanonical(CanonicalPath)) + if (std::error_code EC = makeAbsolute(Path)) return EC; if (Redirection == RedirectKind::Fallback) { // Attempt to find the original file first, only falling back to the // mapped file if that fails. - std::error_code EC = ExternalFS->getRealPath(CanonicalPath, Output); + std::error_code EC = ExternalFS->getRealPath(Path, Output); if (!EC) return EC; } - ErrorOr Result = - lookupPath(CanonicalPath); + ErrorOr Result = lookupPath(Path); if (!Result) { // Was not able to map file, fallthrough to using the original path if // that was the specified redirection type. if (Redirection == RedirectKind::Fallthrough && isFileNotFound(Result.getError())) - return ExternalFS->getRealPath(CanonicalPath, Output); + return ExternalFS->getRealPath(Path, Output); return Result.getError(); } @@ -2571,7 +2569,7 @@ RedirectingFileSystem::getRealPath(const Twine &OriginalPath, // Mapped the file but it wasn't found in the underlying filesystem, // fallthrough to using the original path if that was the specified // redirection type. - return ExternalFS->getRealPath(CanonicalPath, Output); + return ExternalFS->getRealPath(Path, Output); } return P; } diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp index d4abbb4345873..695b09343257f 100644 --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -7,9 +7,11 @@ //===----------------------------------------------------------------------===// #include "llvm/Support/VirtualFileSystem.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/Config/llvm-config.h" #include "llvm/Support/Errc.h" +#include "llvm/Support/FileSystem.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" #include "llvm/Support/SourceMgr.h" @@ -3330,3 +3332,66 @@ TEST(RedirectingFileSystemTest, Used) { EXPECT_TRUE(Redirecting1->hasBeenUsed()); EXPECT_FALSE(Redirecting2->hasBeenUsed()); } + +// Check that paths looked up in the external filesystem are unmodified, except +// potentially to add the working directory. We cannot canonicalize away .. +// in the presence of symlinks in the external filesystem. +TEST(RedirectingFileSystemTest, ExternalPaths) { + struct InterceptorFS : llvm::vfs::ProxyFileSystem { + std::vector SeenPaths; + + InterceptorFS(IntrusiveRefCntPtr UnderlyingFS) + : ProxyFileSystem(UnderlyingFS) {} + + llvm::ErrorOr status(const Twine &Path) override { + SeenPaths.push_back(Path.str()); + return ProxyFileSystem::status(Path); + } + + llvm::ErrorOr> + openFileForRead(const Twine &Path) override { + SeenPaths.push_back(Path.str()); + return ProxyFileSystem::openFileForRead(Path); + } + + std::error_code isLocal(const Twine &Path, bool &Result) override { + SeenPaths.push_back(Path.str()); + return ProxyFileSystem::isLocal(Path, Result); + } + + vfs::directory_iterator dir_begin(const Twine &Dir, + std::error_code &EC) override { + SeenPaths.push_back(Dir.str()); + return ProxyFileSystem::dir_begin(Dir, EC); + } + }; + + std::error_code EC; + auto BaseFS = makeIntrusiveRefCnt(); + BaseFS->setCurrentWorkingDirectory("/cwd"); + auto CheckFS = makeIntrusiveRefCnt(BaseFS); + auto FS = vfs::RedirectingFileSystem::create({}, /*UseExternalNames=*/false, + *CheckFS); + + FS->status("/a/../b"); + FS->openFileForRead("c"); + FS->exists("./d"); + bool IsLocal = false; + FS->isLocal("/e/./../f", IsLocal); + FS->dir_begin(".././g", EC); + + std::vector Expected{"/a/../b", "/cwd/c", "/cwd/./d", + "/e/./../f", "/cwd/.././g"}; + + EXPECT_EQ(CheckFS->SeenPaths, Expected); + + CheckFS->SeenPaths.clear(); + FS->setRedirection(vfs::RedirectingFileSystem::RedirectKind::Fallback); + FS->status("/a/../b"); + FS->openFileForRead("c"); + FS->exists("./d"); + FS->isLocal("/e/./../f", IsLocal); + FS->dir_begin(".././g", EC); + + EXPECT_EQ(CheckFS->SeenPaths, Expected); +}