From 881ba104656c40098d4bc90c52613c08136f0fe1 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Wed, 13 Jun 2018 18:03:14 +0000 Subject: [PATCH] LTO: Keep file handles open for memory mapped files. On Windows we've observed that if you open a file, write to it, map it into memory and close the file handle, the contents of the memory mapping can sometimes be incorrect. That was what we did when adding an entry to the ThinLTO cache using the TempFile and MemoryBuffer classes, and it was causing intermittent build failures on Chromium's ThinLTO bots on Windows. More details are in the associated Chromium bug (crbug.com/786127). We can prevent this from happening by keeping a handle to the file open while the mapping is active. So this patch changes the mapped_file_region class to duplicate the file handle when mapping the file and close it upon unmapping it. One gotcha is that the file handle that we keep open must not have been created with FILE_FLAG_DELETE_ON_CLOSE, as otherwise the operating system will prevent other processes from opening the file. We can achieve this by avoiding the use of FILE_FLAG_DELETE_ON_CLOSE altogether. Instead, we use SetFileInformationByHandle with FileDispositionInfo to manage the delete-on-close bit. This lets us remove the hack that we used to use to clear the delete-on-close bit on a file opened with FILE_FLAG_DELETE_ON_CLOSE. A downside of using SetFileInformationByHandle/FileDispositionInfo as opposed to FILE_FLAG_DELETE_ON_CLOSE is that it prevents us from using CreateFile to open the file while the flag is set, even within the same process. This doesn't seem to matter for almost every client of TempFile, except for LockFileManager, which calls sys::fs::create_link to create a hard link from the lock file, and in the process of doing so tries to open the file. To prevent this change from breaking LockFileManager I changed it to stop using TempFile by effectively reverting r318550. Differential Revision: https://reviews.llvm.org/D48051 llvm-svn: 334630 --- llvm/include/llvm/Support/FileSystem.h | 4 +- llvm/include/llvm/Support/LockFileManager.h | 3 +- llvm/lib/LTO/Caching.cpp | 9 +- llvm/lib/Support/LockFileManager.cpp | 86 ++++++++++++------ llvm/lib/Support/Path.cpp | 8 +- llvm/lib/Support/Unix/Path.inc | 3 +- llvm/lib/Support/Windows/Path.inc | 97 ++++++--------------- 7 files changed, 105 insertions(+), 105 deletions(-) diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h index 0428f837d2a1..af29f9c4927e 100644 --- a/llvm/include/llvm/Support/FileSystem.h +++ b/llvm/include/llvm/Support/FileSystem.h @@ -1043,7 +1043,9 @@ private: /// Platform-specific mapping state. size_t Size; void *Mapping; - int FD; +#ifdef _WIN32 + void *FileHandle; +#endif mapmode Mode; std::error_code init(int FD, uint64_t Offset, mapmode Mode); diff --git a/llvm/include/llvm/Support/LockFileManager.h b/llvm/include/llvm/Support/LockFileManager.h index a5391ae27332..86db0b2b1020 100644 --- a/llvm/include/llvm/Support/LockFileManager.h +++ b/llvm/include/llvm/Support/LockFileManager.h @@ -11,7 +11,6 @@ #include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallString.h" -#include "llvm/Support/FileSystem.h" #include #include // for std::pair @@ -54,7 +53,7 @@ public: private: SmallString<128> FileName; SmallString<128> LockFileName; - Optional UniqueLockFile; + SmallString<128> UniqueLockFileName; Optional > Owner; std::error_code ErrorCode; diff --git a/llvm/lib/LTO/Caching.cpp b/llvm/lib/LTO/Caching.cpp index 22bc82f7ba05..bd6190d99613 100644 --- a/llvm/lib/LTO/Caching.cpp +++ b/llvm/lib/LTO/Caching.cpp @@ -40,7 +40,14 @@ Expected lto::localCache(StringRef CacheDirectoryPath, return AddStreamFn(); } - if (MBOrErr.getError() != errc::no_such_file_or_directory) + // On Windows we can fail to open a cache file with a permission denied + // error. This generally means that another process has requested to delete + // the file while it is still open, but it could also mean that another + // process has opened the file without the sharing permissions we need. + // Since the file is probably being deleted we handle it in the same way as + // if the file did not exist at all. + if (MBOrErr.getError() != errc::no_such_file_or_directory && + MBOrErr.getError() != errc::permission_denied) report_fatal_error(Twine("Failed to open cache file ") + EntryPath + ": " + MBOrErr.getError().message() + "\n"); diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp index 368b276f4b68..77baf7ac4bdd 100644 --- a/llvm/lib/Support/LockFileManager.cpp +++ b/llvm/lib/Support/LockFileManager.cpp @@ -9,10 +9,8 @@ #include "llvm/Support/LockFileManager.h" #include "llvm/ADT/None.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" -#include "llvm/Config/llvm-config.h" #include "llvm/Support/Errc.h" #include "llvm/Support/ErrorOr.h" #include "llvm/Support/FileSystem.h" @@ -123,6 +121,39 @@ bool LockFileManager::processStillExecuting(StringRef HostID, int PID) { return true; } +namespace { + +/// An RAII helper object ensure that the unique lock file is removed. +/// +/// Ensures that if there is an error or a signal before we finish acquiring the +/// lock, the unique file will be removed. And if we successfully take the lock, +/// the signal handler is left in place so that signals while the lock is held +/// will remove the unique lock file. The caller should ensure there is a +/// matching call to sys::DontRemoveFileOnSignal when the lock is released. +class RemoveUniqueLockFileOnSignal { + StringRef Filename; + bool RemoveImmediately; +public: + RemoveUniqueLockFileOnSignal(StringRef Name) + : Filename(Name), RemoveImmediately(true) { + sys::RemoveFileOnSignal(Filename, nullptr); + } + + ~RemoveUniqueLockFileOnSignal() { + if (!RemoveImmediately) { + // Leave the signal handler enabled. It will be removed when the lock is + // released. + return; + } + sys::fs::remove(Filename); + sys::DontRemoveFileOnSignal(Filename); + } + + void lockAcquired() { RemoveImmediately = false; } +}; + +} // end anonymous namespace + LockFileManager::LockFileManager(StringRef FileName) { this->FileName = FileName; @@ -141,22 +172,16 @@ LockFileManager::LockFileManager(StringRef FileName) return; // Create a lock file that is unique to this instance. - Expected Temp = - sys::fs::TempFile::create(LockFileName + "-%%%%%%%%"); - if (!Temp) { - std::error_code EC = errorToErrorCode(Temp.takeError()); - std::string S("failed to create unique file with prefix "); - S.append(LockFileName.str()); + UniqueLockFileName = LockFileName; + UniqueLockFileName += "-%%%%%%%%"; + int UniqueLockFileID; + if (std::error_code EC = sys::fs::createUniqueFile( + UniqueLockFileName, UniqueLockFileID, UniqueLockFileName)) { + std::string S("failed to create unique file "); + S.append(UniqueLockFileName.str()); setError(EC, S); return; } - UniqueLockFile = std::move(*Temp); - - // Make sure we discard the temporary file on exit. - auto RemoveTempFile = llvm::make_scope_exit([&]() { - if (Error E = UniqueLockFile->discard()) - setError(errorToErrorCode(std::move(E))); - }); // Write our process ID to our unique lock file. { @@ -166,46 +191,54 @@ LockFileManager::LockFileManager(StringRef FileName) return; } - raw_fd_ostream Out(UniqueLockFile->FD, /*shouldClose=*/false); + raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true); Out << HostID << ' '; #if LLVM_ON_UNIX Out << getpid(); #else Out << "1"; #endif - Out.flush(); + Out.close(); if (Out.has_error()) { // We failed to write out PID, so report the error, remove the // unique lock file, and fail. std::string S("failed to write to "); - S.append(UniqueLockFile->TmpName); + S.append(UniqueLockFileName.str()); setError(Out.error(), S); + sys::fs::remove(UniqueLockFileName); return; } } + // Clean up the unique file on signal, which also releases the lock if it is + // held since the .lock symlink will point to a nonexistent file. + RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName); + while (true) { // Create a link from the lock file name. If this succeeds, we're done. std::error_code EC = - sys::fs::create_link(UniqueLockFile->TmpName, LockFileName); + sys::fs::create_link(UniqueLockFileName, LockFileName); if (!EC) { - RemoveTempFile.release(); + RemoveUniqueFile.lockAcquired(); return; } if (EC != errc::file_exists) { std::string S("failed to create link "); raw_string_ostream OSS(S); - OSS << LockFileName.str() << " to " << UniqueLockFile->TmpName; + OSS << LockFileName.str() << " to " << UniqueLockFileName.str(); setError(EC, OSS.str()); return; } // Someone else managed to create the lock file first. Read the process ID // from the lock file. - if ((Owner = readLockFile(LockFileName))) - return; // RemoveTempFile will delete out our unique lock file. + if ((Owner = readLockFile(LockFileName))) { + // Wipe out our unique lock file (it's useless now) + sys::fs::remove(UniqueLockFileName); + return; + } if (!sys::fs::exists(LockFileName)) { // The previous owner released the lock file before we could read it. @@ -217,7 +250,7 @@ LockFileManager::LockFileManager(StringRef FileName) // ownership. if ((EC = sys::fs::remove(LockFileName))) { std::string S("failed to remove lockfile "); - S.append(LockFileName.str()); + S.append(UniqueLockFileName.str()); setError(EC, S); return; } @@ -252,7 +285,10 @@ LockFileManager::~LockFileManager() { // Since we own the lock, remove the lock file and our own unique lock file. sys::fs::remove(LockFileName); - consumeError(UniqueLockFile->discard()); + sys::fs::remove(UniqueLockFileName); + // The unique file is now gone, so remove it from the signal handler. This + // matches a sys::RemoveFileOnSignal() in LockFileManager(). + sys::DontRemoveFileOnSignal(UniqueLockFileName); } LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() { diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp index 32c19a0515bf..cc507874b00b 100644 --- a/llvm/lib/Support/Path.cpp +++ b/llvm/lib/Support/Path.cpp @@ -1129,12 +1129,13 @@ Error TempFile::keep(const Twine &Name) { // Always try to close and rename. #ifdef _WIN32 // If we cant't cancel the delete don't rename. - std::error_code RenameEC = cancelDeleteOnClose(FD); + auto H = reinterpret_cast(_get_osfhandle(FD)); + std::error_code RenameEC = setDeleteDisposition(H, false); if (!RenameEC) RenameEC = rename_fd(FD, Name); // If we can't rename, discard the temporary file. if (RenameEC) - removeFD(FD); + setDeleteDisposition(H, true); #else std::error_code RenameEC = fs::rename(TmpName, Name); // If we can't rename, discard the temporary file. @@ -1160,7 +1161,8 @@ Error TempFile::keep() { Done = true; #ifdef _WIN32 - if (std::error_code EC = cancelDeleteOnClose(FD)) + auto H = reinterpret_cast(_get_osfhandle(FD)); + if (std::error_code EC = setDeleteDisposition(H, false)) return errorCodeToError(EC); #else sys::DontRemoveFileOnSignal(TmpName); diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc index b4001fb74817..b8fea5bda4fb 100644 --- a/llvm/lib/Support/Unix/Path.inc +++ b/llvm/lib/Support/Unix/Path.inc @@ -634,8 +634,7 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset, mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length, uint64_t offset, std::error_code &ec) - : Size(length), Mapping(), FD(fd), Mode(mode) { - (void)FD; + : Size(length), Mapping(), Mode(mode) { (void)Mode; ec = init(fd, offset, mode); if (ec) diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index 7ef7468867d6..577d57199d5d 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -404,56 +404,6 @@ static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) { return std::error_code(); } -static std::error_code removeFD(int FD) { - HANDLE Handle = reinterpret_cast(_get_osfhandle(FD)); - return setDeleteDisposition(Handle, true); -} - -/// In order to handle temporary files we want the following properties -/// -/// * The temporary file is deleted on crashes -/// * We can use (read, rename, etc) the temporary file. -/// * We can cancel the delete to keep the file. -/// -/// Using FILE_DISPOSITION_INFO with DeleteFile=true will create a file that is -/// deleted on close, but it has a few problems: -/// -/// * The file cannot be used. An attempt to open or rename the file will fail. -/// This makes the temporary file almost useless, as it cannot be part of -/// any other CreateFileW call in the current or in another process. -/// * It is not atomic. A crash just after CreateFileW or just after canceling -/// the delete will leave the file on disk. -/// -/// Using FILE_FLAG_DELETE_ON_CLOSE solves the first issues and the first part -/// of the second one, but there is no way to cancel it in place. What works is -/// to create a second handle to prevent the deletion, close the first one and -/// then clear DeleteFile with SetFileInformationByHandle. This requires -/// changing the handle and file descriptor the caller uses. -static std::error_code cancelDeleteOnClose(int &FD) { - HANDLE Handle = reinterpret_cast(_get_osfhandle(FD)); - SmallVector Name; - if (std::error_code EC = realPathFromHandle(Handle, Name)) - return EC; - HANDLE NewHandle = - ::CreateFileW(Name.data(), GENERIC_READ | GENERIC_WRITE | DELETE, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); - if (NewHandle == INVALID_HANDLE_VALUE) - return mapWindowsError(::GetLastError()); - if (close(FD)) - return mapWindowsError(::GetLastError()); - - if (std::error_code EC = setDeleteDisposition(NewHandle, false)) - return EC; - - FD = ::_open_osfhandle(intptr_t(NewHandle), 0); - if (FD == -1) { - ::CloseHandle(NewHandle); - return mapWindowsError(ERROR_INVALID_HANDLE); - } - return std::error_code(); -} - static std::error_code rename_internal(HANDLE FromHandle, const Twine &To, bool ReplaceIfExists) { SmallVector ToWide; @@ -826,10 +776,9 @@ std::error_code setLastModificationAndAccessTime(int FD, TimePoint<> Time) { std::error_code mapped_file_region::init(int FD, uint64_t Offset, mapmode Mode) { - this->FD = FD; this->Mode = Mode; - HANDLE FileHandle = reinterpret_cast(_get_osfhandle(FD)); - if (FileHandle == INVALID_HANDLE_VALUE) + HANDLE OrigFileHandle = reinterpret_cast(_get_osfhandle(FD)); + if (OrigFileHandle == INVALID_HANDLE_VALUE) return make_error_code(errc::bad_file_descriptor); DWORD flprotect; @@ -840,7 +789,7 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset, } HANDLE FileMappingHandle = - ::CreateFileMappingW(FileHandle, 0, flprotect, + ::CreateFileMappingW(OrigFileHandle, 0, flprotect, Hi_32(Size), Lo_32(Size), 0); @@ -878,9 +827,20 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset, Size = mbi.RegionSize; } - // Close all the handles except for the view. It will keep the other handles - // alive. + // Close the file mapping handle, as it's kept alive by the file mapping. But + // neither the file mapping nor the file mapping handle keep the file handle + // alive, so we need to keep a reference to the file in case all other handles + // are closed and the file is deleted, which may cause invalid data to be read + // from the file. ::CloseHandle(FileMappingHandle); + if (!::DuplicateHandle(::GetCurrentProcess(), OrigFileHandle, + ::GetCurrentProcess(), &FileHandle, 0, 0, + DUPLICATE_SAME_ACCESS)) { + std::error_code ec = mapWindowsError(GetLastError()); + ::UnmapViewOfFile(Mapping); + return ec; + } + return std::error_code(); } @@ -902,10 +862,10 @@ mapped_file_region::~mapped_file_region() { // flushed and subsequent process's attempts to read a file can return // invalid data. Calling FlushFileBuffers on the write handle is // sufficient to ensure that this bug is not triggered. - HANDLE FileHandle = reinterpret_cast(_get_osfhandle(FD)); - if (FileHandle != INVALID_HANDLE_VALUE) - ::FlushFileBuffers(FileHandle); + ::FlushFileBuffers(FileHandle); } + + ::CloseHandle(FileHandle); } } @@ -1056,16 +1016,6 @@ static std::error_code nativeFileToFd(Expected H, int &ResultFD, return std::error_code(); } -static DWORD nativeOpenFlags(OpenFlags Flags) { - DWORD Result = 0; - if (Flags & OF_Delete) - Result |= FILE_FLAG_DELETE_ON_CLOSE; - - if (Result == 0) - Result = FILE_ATTRIBUTE_NORMAL; - return Result; -} - static DWORD nativeDisposition(CreationDisposition Disp, OpenFlags Flags) { // This is a compatibility hack. Really we should respect the creation // disposition, but a lot of old code relied on the implicit assumption that @@ -1142,7 +1092,6 @@ Expected openNativeFile(const Twine &Name, CreationDisposition Disp, assert((!(Disp == CD_CreateNew) || !(Flags & OF_Append)) && "Cannot specify both 'CreateNew' and 'Append' file creation flags!"); - DWORD NativeFlags = nativeOpenFlags(Flags); DWORD NativeDisp = nativeDisposition(Disp, Flags); DWORD NativeAccess = nativeAccess(Access, Flags); @@ -1152,9 +1101,15 @@ Expected openNativeFile(const Twine &Name, CreationDisposition Disp, file_t Result; std::error_code EC = openNativeFileInternal( - Name, Result, NativeDisp, NativeAccess, NativeFlags, Inherit); + Name, Result, NativeDisp, NativeAccess, FILE_ATTRIBUTE_NORMAL, Inherit); if (EC) return errorCodeToError(EC); + if (Flags & OF_Delete) { + if ((EC = setDeleteDisposition(Result, true))) { + ::CloseHandle(Result); + return errorCodeToError(EC); + } + } return Result; }