From: Martin Storsjö Date: Mon, 4 Oct 2021 12:10:52 +0000 (+0300) Subject: [Support] [Windows] Manually clean up temp files if not setting delete disposition X-Git-Tag: upstream/15.0.7~27385 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=177176f75c6fa3f624d6d964b9d340ce39511565;p=platform%2Fupstream%2Fllvm.git [Support] [Windows] Manually clean up temp files if not setting delete disposition Since D81803 / 79657e2339b58bc01fe1b85a448bb073d57d90bb, temp files created on network shares don't set "Disposition.DeleteFile = true". This flag normally takes care of removing the temp file both if the process exits abnormally (either crashing or killed externally), and when the file is closed cleanly. For network shares, we voluntarily choose to not set the flag, and if the operation to inspect the file handle (as a prerequisite to setting the flag since 79657e2339b58bc01fe1b85a448bb073d57d90bb) fails we also error out. In both of these cases, we can at least make sure to remove the temp files when they are closed cleanly. Adjust the semantics of "OF_Delete" to not set the delete disposition, but only set the access mode for allowing deletion. Move the call to setDeleteDisposition into TempFile::create, where we can check if it failed, and if it did, set a flag noting that the file should be removed manually at the end. This does leak files on crash, but at least doesn't leak files in regular successful runs. (Technically, the alternative codepath could use the RemoveFileOnSignal function, but that might complicate the TempFile implementation further.) This fixes https://github.com/mstorsjo/llvm-mingw/issues/233 and https://bugs.llvm.org/show_bug.cgi?id=52080. Differential Revision: https://reviews.llvm.org/D111875 --- diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h index 38779ef..1a04953 100644 --- a/llvm/include/llvm/Support/FileSystem.h +++ b/llvm/include/llvm/Support/FileSystem.h @@ -772,7 +772,8 @@ enum OpenFlags : unsigned { /// The file should be opened in append mode. OF_Append = 4, - /// Delete the file on close. Only makes a difference on windows. + /// The returned handle can be used for deleting the file. Only makes a + /// difference on windows. OF_Delete = 8, /// When a child process is launched, this file should remain open in the @@ -865,6 +866,11 @@ public: // The open file descriptor. int FD = -1; +#ifdef _WIN32 + // Whether we need to manually remove the file on close. + bool RemoveOnClose = false; +#endif + // Keep this with the given name. Error keep(const Twine &Name); diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp index f770bcc..5e9456c 100644 --- a/llvm/lib/Support/Path.cpp +++ b/llvm/lib/Support/Path.cpp @@ -1188,6 +1188,10 @@ TempFile &TempFile::operator=(TempFile &&Other) { FD = Other.FD; Other.Done = true; Other.FD = -1; +#ifdef _WIN32 + RemoveOnClose = Other.RemoveOnClose; + Other.RemoveOnClose = false; +#endif return *this; } @@ -1202,20 +1206,25 @@ Error TempFile::discard() { FD = -1; #ifdef _WIN32 - // On windows closing will remove the file. - TmpName = ""; - return Error::success(); + // On Windows, closing will remove the file, if we set the delete + // disposition. If not, remove it manually. + bool Remove = RemoveOnClose; #else - // Always try to close and remove. + // Always try to remove the file. + bool Remove = true; +#endif std::error_code RemoveEC; - if (!TmpName.empty()) { + if (Remove && !TmpName.empty()) { RemoveEC = fs::remove(TmpName); +#ifndef _WIN32 sys::DontRemoveFileOnSignal(TmpName); +#endif if (!RemoveEC) TmpName = ""; + } else { + TmpName = ""; } return errorCodeToError(RemoveEC); -#endif } Error TempFile::keep(const Twine &Name) { @@ -1226,19 +1235,26 @@ Error TempFile::keep(const Twine &Name) { // If we can't cancel the delete don't rename. auto H = reinterpret_cast(_get_osfhandle(FD)); std::error_code RenameEC = setDeleteDisposition(H, false); + bool ShouldDelete = false; if (!RenameEC) { RenameEC = rename_handle(H, Name); // If rename failed because it's cross-device, copy instead if (RenameEC == std::error_code(ERROR_NOT_SAME_DEVICE, std::system_category())) { RenameEC = copy_file(TmpName, Name); - setDeleteDisposition(H, true); + ShouldDelete = true; } } - // If we can't rename, discard the temporary file. + // If we can't rename or copy, discard the temporary file. if (RenameEC) - setDeleteDisposition(H, true); + ShouldDelete = true; + if (ShouldDelete) { + if (!RemoveOnClose) + setDeleteDisposition(H, true); + else + remove(TmpName); + } #else std::error_code RenameEC = fs::rename(TmpName, Name); if (RenameEC) { @@ -1295,7 +1311,12 @@ Expected TempFile::create(const Twine &Model, unsigned Mode, return errorCodeToError(EC); TempFile Ret(ResultPath, FD); -#ifndef _WIN32 +#ifdef _WIN32 + auto H = reinterpret_cast(_get_osfhandle(FD)); + if (std::error_code EC = setDeleteDisposition(H, true)) { + Ret.RemoveOnClose = true; + } +#else if (sys::RemoveFileOnSignal(ResultPath)) { // Make sure we delete the file when RemoveFileOnSignal fails. consumeError(Ret.discard()); diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index c1d291731..f4c2628 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -416,8 +416,7 @@ static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) { // Check if the file is on a network (non-local) drive. If so, don't // continue when DeleteFile is true, since it prevents opening the file for - // writes. Note -- this will leak temporary files on disk, but only when the - // target file is on a network drive. + // writes. SmallVector FinalPath; if (std::error_code EC = realPathFromHandle(Handle, FinalPath)) return EC; @@ -427,7 +426,7 @@ static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) { return EC; if (!IsLocal) - return std::error_code(); + return errc::not_supported; // The file is on a local drive, we can safely set FILE_DISPOSITION_INFO's // flag. @@ -1183,12 +1182,6 @@ Expected openNativeFile(const Twine &Name, CreationDisposition Disp, } } - if (Flags & OF_Delete) { - if ((EC = setDeleteDisposition(Result, true))) { - ::CloseHandle(Result); - return errorCodeToError(EC); - } - } return Result; }