[llvm-objcopy] Make .build-id linking atomic

[jakehehrlich]

Summary

This change makes linking into .build-id atomic and safe to use. Some users under particular workflows are reporting that this races more than half the time under particular conditions.

[jhenderson]

Missing context is making it hard for me to review the Path changes.

llvm/include/llvm/Support/FileSystem.h

770

model -> Model, I assume to match the variable name?

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

186

odds a conflict -> odds of a conflict

195

seriouslly -> seriously

202

"to he same"?

Not sure if that should be "to some" or "to the same"

[jakehehrlich]

1. Fixed typos
2. Added context (sorry about that, rookie mistake)

[jhenderson]

llvm/include/llvm/Support/FileSystem.h

770

This still doesn't match the variable name?

772

Rogue \a?

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

205–206

It may depend on the reporting site, but I believe that this will throw away the potentially useful information in EC. I think you need to add EC's message to the string error to avoid this.

[jakehehrlich]

llvm/include/llvm/Support/FileSystem.h

770

Sorry I had a draft comment about this and was waiting to post it once I uploaded context....and then I just didn't; whoops.

The comments were just copied from the documentation below.

[jhenderson]

llvm/include/llvm/Support/FileSystem.h

770

Then the comments below are wrong!

[jakehehrlich]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

205–206

I looked at the implementation of createStringError. It just calls make_error which in turn calls the StringError constructor which does not include the error code unfortunately so your concern was correct. I added a function here to resolve this issue.

[jhenderson]

LGTM. It might be worth getting somebody who has worked more on stuff in the Support library before to review this too, but I'll leave that up to you.

[grimar]

I just do not feel I am a good person to review/accept that, overall looks OK for my eye though,
a minor nit/question/suggestion is inline.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

214

Seems here and above you could use SmallString::c_str()?

It's just implemented very close to this I think:

const char* c_str() {
  this->push_back(0);
  this->pop_back();
  return this->data();
}

[rupprecht]

Just a couple nits, overall lgtm

llvm/lib/Support/Path.cpp

756

Can you preserve the // Null terminate. comment here?

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

168

Why the parens here? This expands to: "%%%%%%%%" "%%%%%%%%" "%%%%%%%%" "%%%%%%%%" which is implicitly joined together. Is it just for clarity, or is it actually required?

Either way -- either MODEL_16 should also be parens'd, or neither should be parens'd.

205–206

Wow, this is kinda a wtf. From Error.cpp:

StringError::StringError(std::error_code EC, const Twine &S)
    : Msg(S.str()), EC(EC) {}

StringError::StringError(const Twine &S, std::error_code EC)
    : Msg(S.str()), EC(EC), PrintMsgOnly(true) {}

void StringError::log(raw_ostream &OS) const {
  if (PrintMsgOnly) {
    OS << Msg;
  } else {
    OS << EC.message();
    if (!Msg.empty())
      OS << (" " + Msg);
  }
}

So, StringError(EC, "foo") will print "error code: foo", but StringError("foo", EC) will just print "foo". createStringError() uses the StringError("foo", EC) constructor.

That seems very strange. I'm totally in favor of creating a makeStringError() method here to workaround that, but we should probably fix that.

214

Why is it even necessary to do this here? Doesn't createUniquePath null terminate it?
Using c_str() seems like a good idea anyway.

[rupprecht]

It looks like rL356404 committed this without referencing the differential revision or addressing some of the comments

[jakehehrlich]

Right sorry about that.

llvm/include/llvm/Support/FileSystem.h

enum OpenFlags : unsigned {
   /// When a child process is launched, this file should remain open in the
   /// child process.
   OF_ChildInherit = 8,
 
   /// Force files Atime to be updated on access. Only makes a difference on windows.
   OF_UpdateAtime = 16,
 };
 
+/// Create a potentially unique file name but does not create it.
+///
+/// Generates a unique path suitable for a temporary file but does not
+/// open or create the file. The name is based on \a Model with '%'
+/// replaced by a random char in [0-9a-f]. If \a MakeAbsolute is true
+/// then the system's temp directory is prepended first. If \a MakeAbsolute
+/// is false the current directory will be used instead.
+///
+/// This function does not check if the file exists. If you want to be sure
+/// that the file does not yet exist, you should use use enough '%' characters
+/// in your model to ensure this. Each '%' gives 4-bits of entropy so you can
+/// use 32 of them to get 128 bits of entropy.
+///
+/// Example: clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s
+///
+/// @param Model Name to base unique path off of.
+/// @param ResultPath Set to the file's path.
+/// @param MakeAbsolute Whether to use the system temp directory.
+void createUniquePath(const Twine &Model, SmallVectorImpl<char> &ResultPath,
+                      bool MakeAbsolute);
+
 /// Create a uniquely named file.
 ///
 /// Generates a unique path suitable for a temporary file and then opens it as a
-/// file. The name is based on \a model with '%' replaced by a random char in
-/// [0-9a-f]. If \a model is not an absolute path, the temporary file will be
+/// file. The name is based on \a Model with '%' replaced by a random char in
+/// [0-9a-f]. If \a Model is not an absolute path, the temporary file will be
 /// created in the current directory.
 ///
 /// Example: clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s
 ///
 /// This is an atomic operation. Either the file is created and opened, or the
 /// file system is left untouched.
 ///
 /// The intended use is for files that are to be kept, possibly after

llvm/lib/Support/Path.cpp

enum FSEntity {
   FS_Name
 };
 
 static std::error_code
 createUniqueEntity(const Twine &Model, int &ResultFD,
                    SmallVectorImpl<char> &ResultPath, bool MakeAbsolute,
                    unsigned Mode, FSEntity Type,
                    sys::fs::OpenFlags Flags = sys::fs::OF_None) {
-  SmallString<128> ModelStorage;
-  Model.toVector(ModelStorage);
-
-  if (MakeAbsolute) {
-    // Make model absolute by prepending a temp directory if it's not already.
-    if (!sys::path::is_absolute(Twine(ModelStorage))) {
-      SmallString<128> TDir;
-      sys::path::system_temp_directory(true, TDir);
-      sys::path::append(TDir, Twine(ModelStorage));
-      ModelStorage.swap(TDir);
-    }
-  }
-
-  // From here on, DO NOT modify model. It may be needed if the randomly chosen
-  // path already exists.
-  ResultPath = ModelStorage;
-  // Null terminate.
-  ResultPath.push_back(0);
-  ResultPath.pop_back();
 
   // Limit the number of attempts we make, so that we don't infinite loop. E.g.
   // "permission denied" could be for a specific file (so we retry with a
   // different name) or for the whole directory (retry would always fail).
   // Checking which is racy, so we try a number of times, then give up.
   std::error_code EC;
   for (int Retries = 128; Retries > 0; --Retries) {
-    // Replace '%' with random chars.
-    for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
-      if (ModelStorage[i] == '%')
-        ResultPath[i] =
-            "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
-    }
-
+    sys::fs::createUniquePath(Model, ResultPath, MakeAbsolute);
     // Try to open + create the file.
     switch (Type) {
     case FS_File: {
       EC = sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD,
                                          sys::fs::CD_CreateNew, Flags, Mode);
       if (EC) {
         // errc::permission_denied happens on Windows when we try to open a file
         // that has been marked for deletion.
std::error_code getUniqueID(const Twine Path, UniqueID &Result) {
   file_status Status;
   std::error_code EC = status(Path, Status);
   if (EC)
     return EC;
   Result = Status.getUniqueID();
   return std::error_code();
 }
 
+void createUniquePath(const Twine &Model, SmallVectorImpl<char> &ResultPath,
+                      bool MakeAbsolute) {
+  SmallString<128> ModelStorage;
+  Model.toVector(ModelStorage);
+
+  if (MakeAbsolute) {
+    // Make model absolute by prepending a temp directory if it's not already.
+    if (!sys::path::is_absolute(Twine(ModelStorage))) {
+      SmallString<128> TDir;
+      sys::path::system_temp_directory(true, TDir);
+      sys::path::append(TDir, Twine(ModelStorage));
+      ModelStorage.swap(TDir);
+    }
+  }
+
+  ResultPath = ModelStorage;
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
+
+  // Replace '%' with random chars.
+  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
+    if (ModelStorage[i] == '%')
+      ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+  }
+}
+
 std::error_code createUniqueFile(const Twine &Model, int &ResultFd,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode) {
   return createUniqueEntity(Model, ResultFd, ResultPath, false, Mode, FS_File);
 }
 
 static std::error_code createUniqueFile(const Twine &Model, int &ResultFd,
                                         SmallVectorImpl<char> &ResultPath,

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

findBuildID(const object::ELFObjectFileBase &In) {
   else if (auto *O = dyn_cast<ELFObjectFile<ELF32BE>>(&In))
     return findBuildID(*O->getELFFile());
   else if (auto *O = dyn_cast<ELFObjectFile<ELF64BE>>(&In))
     return findBuildID(*O->getELFFile());
 
   llvm_unreachable("Bad file format");
 }
 
+template <class... Ts>
+static Error makeStringError(std::error_code EC, const Twine &Msg, Ts&&... Args) {
+  std::string FullMsg = (EC.message() + ": " + Msg).str();
+  return createStringError(EC, FullMsg.c_str(), std::forward<Ts>(Args)...);
+}
+
+#define MODEL_8 "%%%%%%%%"
+#define MODEL_16 MODEL_8 MODEL_8
+#define MODEL_32 (MODEL_16 MODEL_16)
+
 static Error linkToBuildIdDir(const CopyConfig &Config, StringRef ToLink,
                               StringRef Suffix,
                               ArrayRef<uint8_t> BuildIdBytes) {
   SmallString<128> Path = Config.BuildIdLinkDir;
   sys::path::append(Path, llvm::toHex(BuildIdBytes[0], /*LowerCase*/ true));
   if (auto EC = sys::fs::create_directories(Path))
     return createFileError(
         Path.str(),
-        createStringError(EC, "cannot create build ID link directory"));
+        makeStringError(EC, "cannot create build ID link directory"));
 
   sys::path::append(Path,
                     llvm::toHex(BuildIdBytes.slice(1), /*LowerCase*/ true));
   Path += Suffix;
-  if (auto EC = sys::fs::create_hard_link(ToLink, Path)) {
-    // Hard linking failed, try to remove the file first if it exists.
-    if (sys::fs::exists(Path))
-      sys::fs::remove(Path);
-    EC = sys::fs::create_hard_link(ToLink, Path);
-    if (EC)
-      return createStringError(EC, "cannot link %s to %s", ToLink.data(),
-                               Path.data());
+  SmallString<128> TmpPath;
+  // create_hard_link races so we need to link to a temporary path but
+  // we want to make sure that we choose a filename that does not exist.
+  // By using 32 model characters we get 128-bits of entropy. It is
+  // unlikely that this string has ever existed before much less exists
+  // on this disk or in the current working directory.
+  // Additionally we prepend the original Path for debugging but also
+  // because it ensures that we're linking within a directory on the same
+  // partition on the same device which is critical. It has the added
+  // win of yet further decreasing the odds of a conflict.
+  sys::fs::createUniquePath(Twine(Path) + "-" + MODEL_32 + ".tmp", TmpPath,
+                            /*MakeAbsolute*/ false);
+  if (auto EC = sys::fs::create_hard_link(ToLink, TmpPath)) {
+    Path.push_back('\0');
+    return makeStringError(EC, "cannot link %s to %s", ToLink.data(),
   }
+  // We then atomically rename the link into place which will just move the
+  // link. If rename fails something is more seriously wrong so just return
+  // an error.
+  if (auto EC = sys::fs::rename(TmpPath, Path)) {
+    Path.push_back('\0');
+    return makeStringError(EC, "cannot link %s to %s", ToLink.data(),
+                             Path.data());
+  }
+  // If `Path` was already a hard-link to the same underlying file then the
+  // temp file will be left so we need to remove it. Remove will not cause
+  // an error by default if the file is already gone so just blindly remove
+  // it rather than checking.
+  if (auto EC = sys::fs::remove(TmpPath)) {
+    TmpPath.push_back('\0');
+    return makeStringError(EC, "could not remove %s", TmpPath.data());
+  }
   return Error::success();
 }
 
 static Error splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
                             StringRef File, ElfType OutputElfType) {
   auto DWOFile = Reader.create();
   auto OnlyKeepDWOPred = [&DWOFile](const SectionBase &Sec) {
     return onlyKeepDWOPred(*DWOFile, Sec);