From bb22cd1f19d11f85d7136011eaac0f1959e5a59e Mon Sep 17 00:00:00 2001 From: Jake Ehrlich Date: Mon, 18 Mar 2019 20:35:18 +0000 Subject: [PATCH] [llvm-objcopy] Make .build-id linking atomic 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. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@356404 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/FileSystem.h | 25 +++++++++++++++-- lib/Support/Path.cpp | 53 ++++++++++++++++++----------------- tools/llvm-objcopy/ELF/ELFObjcopy.cpp | 52 ++++++++++++++++++++++++++++------ 3 files changed, 93 insertions(+), 37 deletions(-) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index 156250faabe..da12c88078e 100644 --- a/include/llvm/Support/FileSystem.h +++ b/include/llvm/Support/FileSystem.h @@ -764,11 +764,32 @@ enum OpenFlags : unsigned { 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 &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 diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp index 202248c18d8..902df74a68f 100644 --- a/lib/Support/Path.cpp +++ b/lib/Support/Path.cpp @@ -169,25 +169,6 @@ createUniqueEntity(const Twine &Model, int &ResultFD, SmallVectorImpl &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 @@ -195,13 +176,7 @@ createUniqueEntity(const Twine &Model, int &ResultFD, // 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: { @@ -762,6 +737,32 @@ std::error_code getUniqueID(const Twine Path, UniqueID &Result) { return std::error_code(); } +void createUniquePath(const Twine &Model, SmallVectorImpl &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 &ResultPath, unsigned Mode) { diff --git a/tools/llvm-objcopy/ELF/ELFObjcopy.cpp b/tools/llvm-objcopy/ELF/ELFObjcopy.cpp index b859e3a192b..883ffa0aaf6 100644 --- a/tools/llvm-objcopy/ELF/ELFObjcopy.cpp +++ b/tools/llvm-objcopy/ELF/ELFObjcopy.cpp @@ -157,6 +157,16 @@ findBuildID(const object::ELFObjectFileBase &In) { llvm_unreachable("Bad file format"); } +template +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(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 BuildIdBytes) { @@ -165,19 +175,43 @@ static Error linkToBuildIdDir(const CopyConfig &Config, StringRef ToLink, 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(), + Path.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(); } -- 2.11.0