[LLVM-AR] Fixed bug where temporary file would be left behind every time an archive was updated

[cameron314]

Summary

When updating an existing archive, llvm-ar opens the old archive into a MemoryBuffer, does its thing, and writes the results to a temporary file. That file is then renamed to the original archive filename, thus replacing it with the updated contents. However, on Windows at least, what would happen is that the MemoryBuffer for the old archive would actually be an mmap'ed view of the file, so when it came time to do the rename via Win32's ReplaceFile, it would succeed but would be unable to fully replace the file since there would still be a handle open on it; instead, the old version got renamed to a random temporary name and left behind.

This patch fixes the problem by allowing the backing memory buffer to be destroyed after it's no longer needed, just before the rename takes place.

[• rafael]

include/llvm/Object/Archive.h

179 ↗

(On Diff #55901)

We really should not do this.

All these objects just wrap a MemoryBufferRef. It is llvm-ar that should be deleting these buffers earlier.

[silvas]

FWIW Michael and I have noticed llvm-ar leaving behind strange .TMP files created internally to ReplaceFile. Looking forward to trying this out!

[cameron314]

@silvas: Yes, that's exactly what I discovered too while debugging this.

include/llvm/Object/Archive.h

179 ↗

(On Diff #55901)

Ideally, I agree. But the design is such that llvm-ar has no control over when the buffer is done being used. It passes an Archive off to writeArchive which consumes it and does the file replacement in one step. I thought about splitting that function in two, but that's even uglier.

This was the cleanest way of fixing the bug I could come up with.

[silvas]

In D19838#420486, @cameron314 wrote:

@silvas: Yes, that's exactly what I discovered too while debugging this.

Is there a way we can fix this in rename in lib/Support/Windows/Path.inc? I just noticed that clang-tidy has similar issues.

[• rafael]

All these objects just wrap a MemoryBufferRef. It is llvm-ar that should be deleting these buffers earlier.

Ideally, I agree. But the design is such that llvm-ar has no control over when the buffer is done being used. It passes an Archive off to writeArchive which consumes it and does the file replacement in one step. I thought about splitting that function in two, but that's even uglier.

Maybe pass the buffer ownership to writeArchive?

In any case, we really should not have an Archive that can optionally
own the buffer.

Cheers,
Rafael

[cameron314]

@silvas: The problem, unfortunately, is not with the way rename is implemented -- when the destination file has an open file handle on it, there's only so much that can be done regardless of the method.

@rafael: I could do that. It made more sense to me that it be linked to the archive (since once freed, the archive is no longer valid).

[cameron314]

All right, I've changed the patch according to your suggestion. The buffer is no longer linked to the Archive object, and is passed separately. (Note this leaves the Archive object in an unusable state after the buffer is removed from under it, but that llvm-ar doesn't use it after that point.)

Long term, it might be a good idea to introduce another flag to MemoryBuffer::getFile to always force it to copy the file into memory instead of mapping it. This would be less efficient most of the time, but would fix this sort of bug which is otherwise very annoying to handle if there's any abstraction at all in the code that consumes the buffer before overwriting the file. It seems there's a lot of read-modify-update file access idioms across the various tools that are prone to this problem.

[• rafael]

This looks good to me with a few nits:

Expand the comment to describe the windows limitation. I think most people
looking at this code will have POSIX semantics in mind.

Second, is it possible to add a test the currently fails on windows? What
does the renamed file looks like? Is it something we can guess with a glob?

Thanks a lot for fixing this.

Cheers,
Rafael

[cameron314]

I thought this would be an issue on POSIX as well, but after reading the documentation and testing it on a Linux machine it actually isn't -- POSIX allows a file to be renamed to a file that is currently open. This seems really unsafe to me since whoever has the destination file already open is getting the contents on disk changed out from under them (unless my understanding is off) which could cause all sorts of nasty corruption issues if there's a non-empty read or write buffer (or unflushed mapped memory) on that filehandle. (On Windows, a file that is in use can generally be renamed but not deleted, and the open handles keep referring to the same file regardless of its name -- which is why ReplaceFile in this case renames the destination file to let it hang around for whoever has it open.)

I'll expand the comment, good call!

I really don't know how to test this nicely. It depends on whether shouldUseMmap returns true in MemoryBuffer.cpp. The temporary files seem to be of the form "destarcname.a~RFnnnnxxxx.TMP", but this is a Win32 implementation detail and is not documented on MSDN, so is probably subject to change.

[cameron314]

Here's the same diff but with a better and expanded comment.

[• rafael]

LGTM.

Thank you so much for tracking this down.

[• rafael]

Do you have commit access?

Cheers,
Rafael

[cameron314]

Thanks! It was bugging us too ;-)
No I do not. Would you mind committing it for me?

[• rafael]

r268916.

Thank you so much,
Rafael

[cameron314]

Thank you! Closed by rL268916.

include/llvm/Object/ArchiveWriter.h

public:
   StringRef getNew() const;
   llvm::ErrorOr<int> getFD(sys::fs::file_status &NewStatus) const;
   const sys::fs::file_status &getStatus() const;
 };
 
 std::pair<StringRef, std::error_code>
 writeArchive(StringRef ArcName, std::vector<NewArchiveIterator> &NewMembers,
              bool WriteSymtab, object::Archive::Kind Kind, bool Deterministic,
-             bool Thin);
+             bool Thin, std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
 }
 
 #endif

lib/Object/ArchiveWriter.cpp

writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind,
   Out.seek(Pos);
   return BodyStartOffset + 4;
 }
 
 std::pair<StringRef, std::error_code>
 llvm::writeArchive(StringRef ArcName,
                    std::vector<NewArchiveIterator> &NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
-                   bool Deterministic, bool Thin) {
+                   bool Deterministic, bool Thin,
+                   std::unique_ptr<MemoryBuffer> OldArchiveBuf) {
   SmallString<128> TmpArchive;
   int TmpArchiveFD;
   if (auto EC = sys::fs::createUniqueFile(ArcName + ".temp-archive-%%%%%%%.a",
                                           TmpArchiveFD, TmpArchive))
     return std::make_pair(ArcName, EC);
 
   tool_output_file Output(TmpArchive, TmpArchiveFD);
   raw_fd_ostream &Out = Output.os();
for (unsigned MemberNum : MemberOffsetRefs) {
       if (Kind == object::Archive::K_BSD)
         Out.seek(Out.tell() + 4); // skip over the string offset
       print32(Out, Kind, MemberOffset[MemberNum]);
     }
   }
 
   Output.keep();
   Out.close();
+
+  // At this point, we no longer need whatever backing memory
+  // was used to generate the NewMembers. On Windows, this buffer
+  // could be a mapped view of the file we want to replace (if
+  // we're updating an existing archive, say). In that case, the
+  // rename would still succeed, but it would leave behind a
+  // temporary file (actually the original file renamed) because
+  // a file cannot be deleted while there's a handle open on it,
+  // only renamed. So by freeing this buffer, this ensures that
+  // the last open handle on the destination file, if any, is
+  // closed before we attempt to rename.
+  OldArchiveBuf.reset();
+
   sys::fs::rename(TmpArchive, ArcName);
   return std::make_pair("", std::error_code());
 }

tools/llvm-ar/llvm-ar.cpp

for (auto &Member : Members) {
     addMember(Ret, Member, Pos);
     ++Pos;
   }
 
   return Ret;
 }
 
 static void
-performWriteOperation(ArchiveOperation Operation, object::Archive *OldArchive,
+performWriteOperation(ArchiveOperation Operation,
+                      object::Archive *OldArchive,
+                      std::unique_ptr<MemoryBuffer> OldArchiveBuf,
                       std::vector<NewArchiveIterator> *NewMembersP) {
   object::Archive::Kind Kind;
   switch (FormatOpt) {
   case Default: {
     Triple T(sys::getProcessTriple());
     if (T.isOSDarwin())
       Kind = object::Archive::K_BSD;
     else
       Kind = object::Archive::K_GNU;
     break;
   }
   case GNU:
     Kind = object::Archive::K_GNU;
     break;
   case BSD:
     Kind = object::Archive::K_BSD;
     break;
   }
   if (NewMembersP) {
     std::pair<StringRef, std::error_code> Result = writeArchive(
-        ArchiveName, *NewMembersP, Symtab, Kind, Deterministic, Thin);
+        ArchiveName, *NewMembersP, Symtab, Kind, Deterministic, Thin,
+        std::move(OldArchiveBuf));
     failIfError(Result.second, Result.first);
     return;
   }
   std::vector<NewArchiveIterator> NewMembers =
       computeNewArchiveMembers(Operation, OldArchive);
   auto Result =
-      writeArchive(ArchiveName, NewMembers, Symtab, Kind, Deterministic, Thin);
+      writeArchive(ArchiveName, NewMembers, Symtab, Kind, Deterministic, Thin,
+      std::move(OldArchiveBuf));
   failIfError(Result.second, Result.first);
 }
 
 static void createSymbolTable(object::Archive *OldArchive) {
   // When an archive is created or modified, if the s option is given, the
   // resulting archive will have a current symbol table. If the S option
   // is given, it will have no symbol table.
   // In summary, we only need to update the symbol table if we have none.
   // This is actually very common because of broken build systems that think
   // they have to run ranlib.
   if (OldArchive->hasSymbolTable())
     return;
 
-  performWriteOperation(CreateSymTab, OldArchive, nullptr);
+  performWriteOperation(CreateSymTab, OldArchive, nullptr, nullptr);
 }
 
 static void performOperation(ArchiveOperation Operation,
                              object::Archive *OldArchive,
+                             std::unique_ptr<MemoryBuffer> OldArchiveBuf,
                              std::vector<NewArchiveIterator> *NewMembers) {
   switch (Operation) {
   case Print:
   case DisplayTable:
   case Extract:
     performReadOperation(Operation, OldArchive);
     return;
 
   case Delete:
   case Move:
   case QuickAppend:
   case ReplaceOrInsert:
-    performWriteOperation(Operation, OldArchive, NewMembers);
+    performWriteOperation(Operation, OldArchive, std::move(OldArchiveBuf),
+                          NewMembers);
     return;
   case CreateSymTab:
     createSymbolTable(OldArchive);
     return;
   }
   llvm_unreachable("Unknown operation.");
 }
 
 static int performOperation(ArchiveOperation Operation,
                             std::vector<NewArchiveIterator> *NewMembers) {
   // Create or open the archive object.
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf =
       MemoryBuffer::getFile(ArchiveName, -1, false);
   std::error_code EC = Buf.getError();
   if (EC && EC != errc::no_such_file_or_directory)
     fail("error opening '" + ArchiveName + "': " + EC.message() + "!");
 
   if (!EC) {
     object::Archive Archive(Buf.get()->getMemBufferRef(), EC);
     failIfError(EC,
                 "error loading '" + ArchiveName + "': " + EC.message() + "!");
-    performOperation(Operation, &Archive, NewMembers);
+    performOperation(Operation, &Archive, std::move(Buf.get()), NewMembers);
     return 0;
   }
 
   assert(EC == errc::no_such_file_or_directory);
 
   if (!shouldCreateArchive(Operation)) {
     failIfError(EC, Twine("error loading '") + ArchiveName + "'");
   } else {
     if (!Create) {
       // Produce a warning if we should and we're creating the archive
       errs() << ToolName << ": creating " << ArchiveName << "\n";
     }
   }
 
-  performOperation(Operation, nullptr, NewMembers);
+  performOperation(Operation, nullptr, nullptr, NewMembers);
   return 0;
 }
 
 static void runMRIScript() {
   enum class MRICommand { AddLib, AddMod, Create, Save, End, Invalid };
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf = MemoryBuffer::getSTDIN();
   failIfError(Buf.getError());