Fixed a race condition in PrecompiledPreamble.

[ilya-biryukov]

Summary

Two PrecompiledPreambles, used in parallel on separate threads,
could be writing preamble to the same temporary file.

[ilya-biryukov]

Catched by a test from https://reviews.llvm.org/D36261.

[klimek]

lib/Frontend/PrecompiledPreamble.cpp

470–471 ↗

(On Diff #110418)

I don't understand that yet - why does keeping the file open around the EC check change any of the behavior?
Generally, both create the file, right? So only the first one should get no error?

[ilya-biryukov]

lib/Frontend/PrecompiledPreamble.cpp

470–471 ↗

(On Diff #110418)

createTemporaryFile without fd does not create a file, only checks if the it exists, hence the race condition. Do you think a better solution would be to make it create an empty file and close it right away, similarly to what this function does now?

createTemporaryFile with fd does not have that problem, it actually tries to create a file until it succeeds.

[klimek]

LG, as discussed in person, it's probably a good idea to try to get rid of the non-file-creating version, if possible, or at least fix the comments on the functions to make this behavior clear.

cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp

 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Process.h"
 
 using namespace clang;
 
 namespace {
 
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
if (TmpFile)
     return TempPCHFile::createFromCustomPath(TmpFile);
   return TempPCHFile::createInSystemTempDir("preamble", "pch");
 }
 
 llvm::ErrorOr<PrecompiledPreamble::TempPCHFile>
 PrecompiledPreamble::TempPCHFile::createInSystemTempDir(const Twine &Prefix,
                                                         StringRef Suffix) {
   llvm::SmallString<64> File;
-  auto EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, /*ref*/ File);
+  // Using a version of createTemporaryFile with a file descriptor guarantees
+  // that we would never get a race condition in a multi-threaded setting (i.e.,
+  // multiple threads getting the same temporary path).
+  int FD;
+  auto EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, /*ref*/ FD,
+                                               /*ref*/ File);
   if (EC)
     return EC;
+  // We only needed to make sure the file exists, close the file right away.
+  llvm::sys::Process::SafelyCloseFileDescriptor(FD);
   return TempPCHFile(std::move(File).str());
 }
 
 llvm::ErrorOr<PrecompiledPreamble::TempPCHFile>
 PrecompiledPreamble::TempPCHFile::createFromCustomPath(const Twine &Path) {
   return TempPCHFile(Path.str());
 }