[Bug 21681] - Fixed: Memory leak in FileArchive::find()

[grimar]

Summary

There were 5 subclasses of ArchiveLibraryFile in total and two of them required the fix imo.

[ruiu]

File object ownership is complicated, and I'm not sure if this fix is
correct. Leaking objects is a bug, but is this safe? Who keeps the
ownership of archive files until all linking is complete?

[grimar]

In D13061#250927, @ruiu wrote:

File object ownership is complicated, and I'm not sure if this fix is
correct. Leaking objects is a bug, but is this safe? Who keeps the
ownership of archive files until all linking is complete?

All archive files are ownered by LinkingContext. Files are added as nodes. (like that: ctx->getNodes().push_back(std::move(node));)
LinkingContext looks to be alive all time until link complete.

[ruiu]

LGTM

ReaderWriter/ELF/OutputELFWriter.cpp

49–56

Let's return early.

if (_file->hasAtom())
  return nullptr;
...

[grimar]

Review comments addressed

[grimar]

If everything is ok now - could you please commit ? I dont have write permissions yet.

ReaderWriter/ELF/OutputELFWriter.cpp

49–56

Fixed.
Just please note that we should return nullptr if file does not have atoms.

[grimar]

r248525

ReaderWriter/ELF/OutputELFWriter.cpp

DynamicSymbolFile(ELFLinkingContext &ctx, Resolver resolver)
         _ctx(ctx), _resolver(resolver) {}
 
   File *find(StringRef sym, bool dataSymbolOnly) override {
     if (!_file)
       _file.reset(new (_alloc) SymbolFile<ELFT>(_ctx));
 
     assert(!_file->hasAtoms() && "The file shouldn't have atoms yet");
     _resolver(sym, *_file);
-    // If atoms were added - release the file to the caller.
-    return _file->hasAtoms() ? _file.release() : nullptr;
+
+    if (!_file->hasAtoms())
+      return nullptr;
+
+    // If atoms were added - return the file but also store it for later
+    // destruction.
+    File *result = _file.get();
+    _returnedFiles.push_back(std::move(_file));
+    return result;
   }
 
 private:
   ELFLinkingContext &_ctx;
   Resolver _resolver;
 
   // The allocator should go before bump pointers because of
   // reversed destruction order.
   llvm::BumpPtrAllocator _alloc;
   unique_bump_ptr<SymbolFile<ELFT>> _file;
+  std::vector<unique_bump_ptr<SymbolFile<ELFT>>> _returnedFiles;
 };
 
 } // end anon namespace
 
 template <class ELFT>
 OutputELFWriter<ELFT>::OutputELFWriter(ELFLinkingContext &ctx,
                                        TargetLayout<ELFT> &layout)
     : _ctx(ctx), _targetHandler(ctx.getTargetHandler()), _layout(layout) {}

ReaderWriter/FileArchive.cpp

 //===- lib/ReaderWriter/FileArchive.cpp -----------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "lld/Driver/Driver.h"
 #include "lld/Core/ArchiveLibraryFile.h"
 #include "lld/Core/LLVM.h"
 #include "lld/Core/LinkingContext.h"
 #include "lld/Core/Parallel.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/ObjectFile.h"
// Check if a file is preloaded.
         return future->get();
       }
     }
 
     std::unique_ptr<File> result;
     if (instantiateMember(ci, result))
       return nullptr;
 
-    // give up the pointer so that this object no longer manages it
-    return result.release();
+    File *file = result.get();
+    _filesReturned.push_back(std::move(result));
+
+    // Give up the file pointer. It was stored and will be destroyed with destruction of FileArchive
+    return file;
   }
 
   // Instantiate a member file containing a given symbol name.
   void preload(TaskGroup &group, StringRef name) override {
     auto member = _symbolMemberMap.find(name);
     if (member == _symbolMemberMap.end())
       return;
     Archive::child_iterator ci = member->second;
private:
   const Registry &_registry;
   std::unique_ptr<Archive> _archive;
   MemberMap _symbolMemberMap;
   InstantiatedSet _membersInstantiated;
   bool _logLoading;
   std::vector<std::unique_ptr<MemoryBuffer>> _memberBuffers;
   std::map<const char *, std::unique_ptr<Future<File *>>> _preloaded;
   std::mutex _mutex;
+  FileVector _filesReturned;
 };
 
 class ArchiveReader : public Reader {
 public:
   ArchiveReader(bool logLoading) : _logLoading(logLoading) {}
 
   bool canParse(file_magic magic, MemoryBufferRef) const override {
     return magic == llvm::sys::fs::file_magic::archive;