[llvm] 177176f - [Support] [Windows] Manually clean up temp files if not setting delete disposition
Martin Storsjö via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 28 00:48:02 PDT 2021
Author: Martin Storsjö
Date: 2021-10-28T10:33:37+03:00
New Revision: 177176f75c6fa3f624d6d964b9d340ce39511565
URL: https://github.com/llvm/llvm-project/commit/177176f75c6fa3f624d6d964b9d340ce39511565
DIFF: https://github.com/llvm/llvm-project/commit/177176f75c6fa3f624d6d964b9d340ce39511565.diff
LOG: [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
Added:
Modified:
llvm/include/llvm/Support/FileSystem.h
llvm/lib/Support/Path.cpp
llvm/lib/Support/Windows/Path.inc
Removed:
################################################################################
diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index 38779ef4a3aff..1a049533b82b1 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
diff erence on windows.
+ /// The returned handle can be used for deleting the file. Only makes a
+ ///
diff erence on windows.
OF_Delete = 8,
/// When a child process is launched, this file should remain open in the
@@ -865,6 +866,11 @@ class TempFile {
// 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 f770bcc548139..5e9456c0ca58c 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<HANDLE>(_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> TempFile::create(const Twine &Model, unsigned Mode,
return errorCodeToError(EC);
TempFile Ret(ResultPath, FD);
-#ifndef _WIN32
+#ifdef _WIN32
+ auto H = reinterpret_cast<HANDLE>(_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 c1d291731a88d..f4c2628b1a55d 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<wchar_t, 128> 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<file_t> openNativeFile(const Twine &Name, CreationDisposition Disp,
}
}
- if (Flags & OF_Delete) {
- if ((EC = setDeleteDisposition(Result, true))) {
- ::CloseHandle(Result);
- return errorCodeToError(EC);
- }
- }
return Result;
}
More information about the llvm-commits
mailing list