Delete temp file if rename fails

[• rafael]

Summary

James noticed that when lld failed to replace the output file it would leave the temporary behind. The problem is that the existing logic is

• cancel the delete flag
• rename

We have to cancel first to avoid renaming and then crashing and deleting the old version. What is missing then is deleting the temporary file if the rename fails.

This can be an issue on both unix and windows, but I am not sure how to cause the rename to fail reliably on unix. I think it can be done on ZFS since it has an ACL system similar to what windows uses, but adding support for checking that in llvm-lit is probably not worth it.

[jhenderson]

LGTM. I confirmed that it works for the case I saw with LLD too.

Is there a particular reason why you're using icacls instead of chmod (available in the GnuWin32 tools) for the test? I'd say it's more familiar to most people, but I'm not bothered either way.

[• rafael]

r319786

lib/Support/Path.cpp

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);
+  // If we can't rename, discard the temporary file.
+  if (RenameEC)
+    removeFD(FD);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
+  // If we can't rename, discard the temporary file.
+  if (RenameEC)
+    remove(TmpName);
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
 
   if (!RenameEC)
     TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());

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);
 }
 
+static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
+  FILE_DISPOSITION_INFO Disposition;
+  Disposition.DeleteFile = Delete;
+  if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
+                                  sizeof(Disposition)))
+    return mapWindowsError(::GetLastError());
+  return std::error_code();
+}
+
+static std::error_code removeFD(int FD) {
+  HANDLE Handle = reinterpret_cast<HANDLE>(_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:
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());
+  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();
 }
 

test/tools/llvm-objcopy/cannot-delete-dest.test

+# REQUIRES: system-windows
+# RUN: icacls %t /grant Everyone:(DC) || true
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
+# RUN: yaml2obj %s > test.o
+# RUN: cp test.o test2.o
+# RUN: icacls test2.o /deny Everyone:(D)
+# RUN: icacls . /deny Everyone:(DC)
+
+# This fails because it cannot replace test2.o
+# RUN: not llvm-objcopy test.o test2.o
+
+# But it doesn't leave any temporary files behind.
+# RUN: not ls test2.o.tmp*
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64