[llvm] r356404 - [llvm-objcopy] Make .build-id linking atomic
Jake Ehrlich via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 18 13:35:18 PDT 2019
Author: jakehehrlich
Date: Mon Mar 18 13:35:18 2019
New Revision: 356404
URL: http://llvm.org/viewvc/llvm-project?rev=356404&view=rev
Log:
[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.
Modified:
llvm/trunk/include/llvm/Support/FileSystem.h
llvm/trunk/lib/Support/Path.cpp
llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
Modified: llvm/trunk/include/llvm/Support/FileSystem.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=356404&r1=356403&r2=356404&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/FileSystem.h (original)
+++ llvm/trunk/include/llvm/Support/FileSystem.h Mon Mar 18 13:35:18 2019
@@ -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<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
Modified: llvm/trunk/lib/Support/Path.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=356404&r1=356403&r2=356404&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Path.cpp (original)
+++ llvm/trunk/lib/Support/Path.cpp Mon Mar 18 13:35:18 2019
@@ -169,25 +169,6 @@ createUniqueEntity(const Twine &Model, i
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
@@ -195,13 +176,7 @@ createUniqueEntity(const Twine &Model, i
// 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
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) {
Modified: llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp?rev=356404&r1=356403&r2=356404&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp Mon Mar 18 13:35:18 2019
@@ -157,6 +157,16 @@ findBuildID(const object::ELFObjectFileB
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) {
@@ -165,19 +175,43 @@ static Error linkToBuildIdDir(const Copy
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();
}
More information about the llvm-commits
mailing list