LTO: Fix a potential race condition in the caching API.

pcc · pcc · commit 128423f99bec · 2017-03-17T00:34:07.000Z
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 llvm-svn: 298020
diff --git a/lld/ELF/LTO.cpp b/lld/ELF/LTO.cpp
@@ -150,10 +150,11 @@ std::vector<InputFile *> BitcodeCompiler::compile() {
   // specified, configure LTO to use it as the cache directory.
   lto::NativeObjectCache Cache;
   if (!Config->ThinLTOCacheDir.empty())
-    Cache = check(lto::localCache(
-        Config->ThinLTOCacheDir, [&](size_t Task, StringRef Path) {
-          Files[Task] = check(MemoryBuffer::getFile(Path));
-        }));
+    Cache = check(
+        lto::localCache(Config->ThinLTOCacheDir,
+                        [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+                          Files[Task] = std::move(MB);
+                        }));
 
   checkError(LTOObj->run(
       [&](size_t Task) {
diff --git a/llvm/include/llvm/LTO/Caching.h b/llvm/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<void(unsigned Task, StringRef Path)> 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<void(unsigned Task, std::unique_ptr<MemoryBuffer> 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<NativeObjectCache> localCache(StringRef CacheDirectoryPath,
-                                       AddFileFn AddFile);
+                                       AddBufferFn AddBuffer);
 
 } // namespace lto
 } // namespace llvm
diff --git a/llvm/lib/LTO/Caching.cpp b/llvm/lib/LTO/Caching.cpp
@@ -22,42 +22,57 @@ using namespace llvm;
 using namespace llvm::lto;
 
 Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
-                                            AddFileFn AddFile) {
+                                            AddBufferFn AddBuffer) {
   if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPath))
     return errorCodeToError(EC);
 
   return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     // 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<std::unique_ptr<MemoryBuffer>> 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<raw_pwrite_stream> OS, AddFileFn AddFile,
+      CacheStream(std::unique_ptr<raw_pwrite_stream> 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<std::unique_ptr<MemoryBuffer>> 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<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
       // This CacheStream will move the temporary file into the cache when done.
       return llvm::make_unique<CacheStream>(
           llvm::make_unique<raw_fd_ostream>(TempFD, /* ShouldClose */ true),
-          AddFile, TempFilename.str(), EntryPath.str(), Task);
+          AddBuffer, TempFilename.str(), EntryPath.str(), Task);
     };
   };
 }
diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp
@@ -831,11 +831,15 @@ static ld_plugin_status allSymbolsReadHook() {
         llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
   };
 
-  auto AddFile = [&](size_t Task, StringRef Path) { Filenames[Task] = Path; };
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> 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/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -275,18 +275,13 @@ int main(int argc, char **argv) {
     return llvm::make_unique<lto::NativeObjectStream>(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<MemoryBuffer> 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");
 }
