Use FILE_FLAG_DELETE_ON_CLOSE for TempFile on windows

[• rafael]

Summary

With this llvm should never leave a temporary file behind.

[• rafael]

Rebase now that it doesn't depend on the LockFileManager change.

[rnk]

lib/Support/Windows/Path.inc

403–405

The atomicity concern didn't bother me that much. There are two windows where the temp file may remain: when the process dies between CreateFileW and the SetFileInformationByHandle Delete=true call, and when the process dies between SetFileInformationByHandle Delete=false and the file rename. It's highly unlikely that we will crash in either of these places. There would have to be an external source of raciness (a second thread or external process killing the process) to cause this bug. It doesn't seem like a show-stopper. The FILE_FLAG_DELETE_ON_CLOSE solution doesn't close the second window.

Can you elaborate on the usability problem, since it's more important? If we tolerate the raciness above, not being able to rename a temporary file isn't such a big deal as long as the TempFile abstraction turns off the delete-on-close disposition before renaming. Do we have uses of TempFile that want to be able to pass the path to some other open() call?

I think it simplifies the code to use separate calls to set the file disposition, if that's possible. Then we don't need to change the user FD and handle all as many error edge cases.

407–411

Is this something you figured out through experimentation or is there some reference, documentation, stack overflow post, etc that explains this technique? It's an awfully convoluted way to get the obviously desirable behavior. :)

423–424

I think we should call ::close(FD) here instead to avoid leaking the FD table entry in the CRT. Stack Overflow suggests that there is no way to close an FD without calling CloseHandle on the underlying handle. I suspect you can still use GetLastError, but I can't find any docs to confirm that.

[• rafael]

Use close. Expand comment.

[rnk]

Cool, looks good. :)

[• rafael]

r319137

include/llvm/Support/FileSystem.h

enum OpenFlags : unsigned {
 
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
   F_Text = 4,
 
   /// Open the file for read and write.
   F_RW = 8,
 
-  /// The returned handle can be used for deleting the file. Only makes a
-  /// difference on windows.
+  /// Delete the file on close. Only makes a difference on windows.
   F_Delete = 16
 };
 
 /// @brief 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

lib/Support/Path.cpp

TempFile &TempFile::operator=(TempFile &&Other) {
   Other.Done = true;
   return *this;
 }
 
 TempFile::~TempFile() { assert(Done); }
 
 Error TempFile::discard() {
   Done = true;
-  // Always try to close and remove.
   std::error_code RemoveEC;
+// On windows closing will remove the file.
+#ifndef LLVM_ON_WIN32
+  // Always try to close and remove.
   if (!TmpName.empty()) {
     RemoveEC = fs::remove(TmpName);
     sys::DontRemoveFileOnSignal(TmpName);
   }
+#endif
 
   if (!RemoveEC)
     TmpName = "";
 
   if (FD != -1 && close(FD) == -1) {
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return errorCodeToError(RemoveEC);
 }
 
 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
+#ifdef LLVM_ON_WIN32
+  // If we cant't cancel the delete don't rename.
+  std::error_code RenameEC = cancelDeleteOnClose(FD);
+  if (!RenameEC)
+    RenameEC = rename_fd(FD, Name);
+#else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   sys::DontRemoveFileOnSignal(TmpName);
+#endif
 
   if (!RenameEC)
     TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return errorCodeToError(RenameEC);
 }
 
 Error TempFile::keep() {
   assert(!Done);
   Done = true;
 
+#ifdef LLVM_ON_WIN32
+  if (std::error_code EC = cancelDeleteOnClose(FD))
+    return errorCodeToError(EC);
+#else
   sys::DontRemoveFileOnSignal(TmpName);
+#endif
+
   TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return Error::success();
 }
 
 Expected<TempFile> TempFile::create(const Twine &Model, unsigned Mode) {
   int FD;
   SmallString<128> ResultPath;
-  if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Mode))
+  if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Mode,
+                                            sys::fs::F_RW | sys::fs::F_Delete))
     return errorCodeToError(EC);
 
-  // Make sure we delete the file when RemoveFileOnSignal fails.
   TempFile Ret(ResultPath, FD);
+#ifndef LLVM_ON_WIN32
   if (sys::RemoveFileOnSignal(ResultPath)) {
+    // Make sure we delete the file when RemoveFileOnSignal fails.
     consumeError(Ret.discard());
     std::error_code EC(errc::operation_not_permitted);
     return errorCodeToError(EC);
   }
+#endif
   return std::move(Ret);
 }
 }
 
 namespace path {
 
 bool user_cache_directory(SmallVectorImpl<char> &Result, const Twine &Path1,
                           const Twine &Path2, const Twine &Path3) {

lib/Support/Windows/Path.inc

std::error_code is_local(int FD, bool &Result) {
   HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
 
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   return is_local_internal(FinalPath, Result);
 }
 
+/// 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<HANDLE>(_get_osfhandle(FD));
+  SmallVector<wchar_t, MAX_PATH> 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());
+
+  FILE_DISPOSITION_INFO Disposition;
+  Disposition.DeleteFile = false;
+  if (!SetFileInformationByHandle(NewHandle, FileDispositionInfo, &Disposition,
+                                  sizeof(Disposition)))
+    return mapWindowsError(::GetLastError());
+  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<wchar_t, 0> ToWide;
   if (auto EC = widenPath(To, ToWide))
     return EC;
 
   std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) - sizeof(wchar_t) +
                                   (ToWide.size() * sizeof(wchar_t)));
for (unsigned Retry = 0; Retry != 200; ++Retry) {
     // process might have raced with us to create and open the destination
     // file, so we need to keep doing this until we succeed.
   }
 
   // The most likely root cause.
   return errc::permission_denied;
 }
 
+static std::error_code rename_fd(int FromFD, const Twine &To) {
+  HANDLE FromHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FromFD));
+  return rename_handle(FromHandle, To);
+}
+
 std::error_code rename(const Twine &From, const Twine &To) {
   // Convert to utf-16.
   SmallVector<wchar_t, 128> WideFrom;
   if (std::error_code EC = widenPath(From, WideFrom))
     return EC;
 
   ScopedFileHandle FromHandle;
   // Retry this a few times to defeat badly behaved file system scanners.
std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
   if (Flags & F_Excl)
     CreationDisposition = CREATE_NEW;
   else if (Flags & F_Append)
     CreationDisposition = OPEN_ALWAYS;
   else
     CreationDisposition = CREATE_ALWAYS;
 
   DWORD Access = GENERIC_WRITE;
+  DWORD Attributes = FILE_ATTRIBUTE_NORMAL;
   if (Flags & F_RW)
     Access |= GENERIC_READ;
-  if (Flags & F_Delete)
+  if (Flags & F_Delete) {
     Access |= DELETE;
+    Attributes |= FILE_FLAG_DELETE_ON_CLOSE;
+  }
 
   HANDLE H =
-      ::CreateFileW(PathUTF16.begin(), Access,
+      ::CreateFileW(PathUTF16.data(), Access,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                    NULL, CreationDisposition, FILE_ATTRIBUTE_NORMAL, NULL);
+                    NULL, CreationDisposition, Attributes, NULL);
 
   if (H == INVALID_HANDLE_VALUE) {
     DWORD LastError = ::GetLastError();
     std::error_code EC = mapWindowsError(LastError);
     // Provide a better error message when trying to open directories.
     // This only runs if we failed to open the file, so there is probably
     // no performances issues.
     if (LastError != ERROR_ACCESS_DENIED)