From ec179d780b28f569a46cef73eeef4c8d09463b00 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Fri, 17 Mar 2017 00:34:07 +0000 Subject: [PATCH] LTO: Fix a potential race condition in the caching API. After the call to sys::fs::exists succeeds, indicating a cache hit, we call AddFile and the client will open the file using the supplied path. If the client is using cache pruning, there is a potential race between the pruner and the client. To avoid this, change the caching API so that it provides a MemoryBuffer to the client, and have clients use that MemoryBuffer where possible. This scheme won't work with the gold plugin because the plugin API expects a file path. So we have the gold plugin use the buffer identifier as a path and live with the race for now. (Note that the gold plugin isn't actually affected by the problem at the moment because it doesn't support cache pruning.) This effectively reverts r279883 modulo the change to use the existing path in the gold plugin. Differential Revision: https://reviews.llvm.org/D31063 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@298020 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/LTO/Caching.h | 11 ++++++++--- lib/LTO/Caching.cpp | 33 ++++++++++++++++++++++++--------- tools/gold/gold-plugin.cpp | 8 ++++++-- tools/llvm-lto2/llvm-lto2.cpp | 11 +++-------- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/include/llvm/LTO/Caching.h b/include/llvm/LTO/Caching.h index fe18009b14c..f5ec70e081c 100644 --- a/include/llvm/LTO/Caching.h +++ b/include/llvm/LTO/Caching.h @@ -24,14 +24,19 @@ namespace lto { /// This type defines the callback to add a pre-existing native object file /// (e.g. in a cache). /// -/// File callbacks must be thread safe. -typedef std::function AddFileFn; +/// MB->getBufferIdentifier() is a valid path for the file at the time that it +/// was opened, but clients should prefer to access MB directly in order to +/// avoid a potential race condition. +/// +/// Buffer callbacks must be thread safe. +typedef std::function MB)> + AddBufferFn; /// Create a local file system cache which uses the given cache directory and /// file callback. This function also creates the cache directory if it does not /// already exist. Expected localCache(StringRef CacheDirectoryPath, - AddFileFn AddFile); + AddBufferFn AddBuffer); } // namespace lto } // namespace llvm diff --git a/lib/LTO/Caching.cpp b/lib/LTO/Caching.cpp index b7542049dbf..d8b91c48ee3 100644 --- a/lib/LTO/Caching.cpp +++ b/lib/LTO/Caching.cpp @@ -22,7 +22,7 @@ using namespace llvm; using namespace llvm::lto; Expected lto::localCache(StringRef CacheDirectoryPath, - AddFileFn AddFile) { + AddBufferFn AddBuffer) { if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPath)) return errorCodeToError(EC); @@ -30,34 +30,49 @@ Expected lto::localCache(StringRef CacheDirectoryPath, // First, see if we have a cache hit. SmallString<64> EntryPath; sys::path::append(EntryPath, CacheDirectoryPath, Key); - if (sys::fs::exists(EntryPath)) { - AddFile(Task, EntryPath); + ErrorOr> MBOrErr = + MemoryBuffer::getFile(EntryPath); + if (MBOrErr) { + AddBuffer(Task, std::move(*MBOrErr)); return AddStreamFn(); } + if (MBOrErr.getError() != std::errc::no_such_file_or_directory) + report_fatal_error(Twine("Failed to open cache file ") + EntryPath + + ": " + MBOrErr.getError().message() + "\n"); + // This native object stream is responsible for commiting the resulting - // file to the cache and calling AddFile to add it to the link. + // file to the cache and calling AddBuffer to add it to the link. struct CacheStream : NativeObjectStream { - AddFileFn AddFile; + AddBufferFn AddBuffer; std::string TempFilename; std::string EntryPath; unsigned Task; - CacheStream(std::unique_ptr OS, AddFileFn AddFile, + CacheStream(std::unique_ptr OS, AddBufferFn AddBuffer, std::string TempFilename, std::string EntryPath, unsigned Task) - : NativeObjectStream(std::move(OS)), AddFile(std::move(AddFile)), + : NativeObjectStream(std::move(OS)), AddBuffer(std::move(AddBuffer)), TempFilename(std::move(TempFilename)), EntryPath(std::move(EntryPath)), Task(Task) {} ~CacheStream() { + // FIXME: This code could race with the cache pruner, but it is unlikely + // that the cache pruner will choose to remove a newly created file. + // Make sure the file is closed before committing it. OS.reset(); // This is atomic on POSIX systems. if (auto EC = sys::fs::rename(TempFilename, EntryPath)) report_fatal_error(Twine("Failed to rename temporary file ") + TempFilename + ": " + EC.message() + "\n"); - AddFile(Task, EntryPath); + + ErrorOr> MBOrErr = + MemoryBuffer::getFile(EntryPath); + if (!MBOrErr) + report_fatal_error(Twine("Failed to open cache file ") + EntryPath + + ": " + MBOrErr.getError().message() + "\n"); + AddBuffer(Task, std::move(*MBOrErr)); } }; @@ -77,7 +92,7 @@ Expected lto::localCache(StringRef CacheDirectoryPath, // This CacheStream will move the temporary file into the cache when done. return llvm::make_unique( llvm::make_unique(TempFD, /* ShouldClose */ true), - AddFile, TempFilename.str(), EntryPath.str(), Task); + AddBuffer, TempFilename.str(), EntryPath.str(), Task); }; }; } diff --git a/tools/gold/gold-plugin.cpp b/tools/gold/gold-plugin.cpp index 123d90df890..38674d20717 100644 --- a/tools/gold/gold-plugin.cpp +++ b/tools/gold/gold-plugin.cpp @@ -831,11 +831,15 @@ static ld_plugin_status allSymbolsReadHook() { llvm::make_unique(FD, true)); }; - auto AddFile = [&](size_t Task, StringRef Path) { Filenames[Task] = Path; }; + auto AddBuffer = [&](size_t Task, std::unique_ptr MB) { + // Note that this requires that the memory buffers provided to AddBuffer are + // backed by a file. + Filenames[Task] = MB->getBufferIdentifier(); + }; NativeObjectCache Cache; if (!options::cache_dir.empty()) - Cache = check(localCache(options::cache_dir, AddFile)); + Cache = check(localCache(options::cache_dir, AddBuffer)); check(Lto->run(AddStream, Cache)); diff --git a/tools/llvm-lto2/llvm-lto2.cpp b/tools/llvm-lto2/llvm-lto2.cpp index 3fe0487476f..ce0f4b98425 100644 --- a/tools/llvm-lto2/llvm-lto2.cpp +++ b/tools/llvm-lto2/llvm-lto2.cpp @@ -275,18 +275,13 @@ int main(int argc, char **argv) { return llvm::make_unique(std::move(S)); }; - auto AddFile = [&](size_t Task, StringRef Path) { - auto ReloadedBufferOrErr = MemoryBuffer::getFile(Path); - if (auto EC = ReloadedBufferOrErr.getError()) - report_fatal_error(Twine("Can't reload cached file '") + Path + "': " + - EC.message() + "\n"); - - *AddStream(Task)->OS << (*ReloadedBufferOrErr)->getBuffer(); + auto AddBuffer = [&](size_t Task, std::unique_ptr MB) { + *AddStream(Task)->OS << MB->getBuffer(); }; NativeObjectCache Cache; if (!CacheDir.empty()) - Cache = check(localCache(CacheDir, AddFile), "failed to create cache"); + Cache = check(localCache(CacheDir, AddBuffer), "failed to create cache"); check(Lto.run(AddStream, Cache), "LTO::run failed"); } -- 2.50.1