From 0c06716f89047e6b1ce64540ef8239a1d04b451f Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Mon, 9 May 2016 13:31:11 +0000 Subject: [PATCH] Fix bug where temporary file would be left behind every time an archive was updated. 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. Patch by Cameron! git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@268916 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Object/ArchiveWriter.h | 2 +- lib/Object/ArchiveWriter.cpp | 16 +++++++++++++++- tools/llvm-ar/llvm-ar.cpp | 20 +++++++++++++------- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/include/llvm/Object/ArchiveWriter.h b/include/llvm/Object/ArchiveWriter.h index b5d2ba35808..a3cc64b5216 100644 --- a/include/llvm/Object/ArchiveWriter.h +++ b/include/llvm/Object/ArchiveWriter.h @@ -42,7 +42,7 @@ public: std::pair writeArchive(StringRef ArcName, std::vector &NewMembers, bool WriteSymtab, object::Archive::Kind Kind, bool Deterministic, - bool Thin); + bool Thin, std::unique_ptr OldArchiveBuf = nullptr); } #endif diff --git a/lib/Object/ArchiveWriter.cpp b/lib/Object/ArchiveWriter.cpp index 1ae14f24178..93e285cf674 100644 --- a/lib/Object/ArchiveWriter.cpp +++ b/lib/Object/ArchiveWriter.cpp @@ -307,7 +307,8 @@ std::pair llvm::writeArchive(StringRef ArcName, std::vector &NewMembers, bool WriteSymtab, object::Archive::Kind Kind, - bool Deterministic, bool Thin) { + bool Deterministic, bool Thin, + std::unique_ptr OldArchiveBuf) { assert((!Thin || Kind == object::Archive::K_GNU) && "Only the gnu format has a thin mode"); SmallString<128> TmpArchive; @@ -443,6 +444,19 @@ llvm::writeArchive(StringRef ArcName, 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()); } diff --git a/tools/llvm-ar/llvm-ar.cpp b/tools/llvm-ar/llvm-ar.cpp index 45f778b9af6..375b34d9b41 100644 --- a/tools/llvm-ar/llvm-ar.cpp +++ b/tools/llvm-ar/llvm-ar.cpp @@ -576,7 +576,9 @@ computeNewArchiveMembers(ArchiveOperation Operation, } static void -performWriteOperation(ArchiveOperation Operation, object::Archive *OldArchive, +performWriteOperation(ArchiveOperation Operation, + object::Archive *OldArchive, + std::unique_ptr OldArchiveBuf, std::vector *NewMembersP) { object::Archive::Kind Kind; switch (FormatOpt) { @@ -599,14 +601,16 @@ performWriteOperation(ArchiveOperation Operation, object::Archive *OldArchive, } if (NewMembersP) { std::pair 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 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); } @@ -620,11 +624,12 @@ static void createSymbolTable(object::Archive *OldArchive) { if (OldArchive->hasSymbolTable()) return; - performWriteOperation(CreateSymTab, OldArchive, nullptr); + performWriteOperation(CreateSymTab, OldArchive, nullptr, nullptr); } static void performOperation(ArchiveOperation Operation, object::Archive *OldArchive, + std::unique_ptr OldArchiveBuf, std::vector *NewMembers) { switch (Operation) { case Print: @@ -637,7 +642,8 @@ static void performOperation(ArchiveOperation Operation, case Move: case QuickAppend: case ReplaceOrInsert: - performWriteOperation(Operation, OldArchive, NewMembers); + performWriteOperation(Operation, OldArchive, std::move(OldArchiveBuf), + NewMembers); return; case CreateSymTab: createSymbolTable(OldArchive); @@ -659,7 +665,7 @@ static int performOperation(ArchiveOperation Operation, 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; } @@ -674,7 +680,7 @@ static int performOperation(ArchiveOperation Operation, } } - performOperation(Operation, nullptr, NewMembers); + performOperation(Operation, nullptr, nullptr, NewMembers); return 0; } -- 2.50.1