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

[Rui Ueyama via llvm-commits]

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?

On Tue, Sep 22, 2015 at 11:06 AM, George Rimar <grimar at accesssoftek.com>
wrote:

> grimar created this revision.
> grimar added a reviewer: ruiu.
> grimar added subscribers: llvm-commits, grimar.
> grimar added a project: lld.
>
> There were 5 subclasses of ArchiveLibraryFile in total and two of them
> required the fix imo.
>
> http://reviews.llvm.org/D13061
>
> Files:
>   ReaderWriter/ELF/OutputELFWriter.cpp
>   ReaderWriter/FileArchive.cpp
>
> Index: ReaderWriter/FileArchive.cpp
> ===================================================================
> --- ReaderWriter/FileArchive.cpp
> +++ ReaderWriter/FileArchive.cpp
> @@ -7,6 +7,7 @@
>  //
>
>  //===----------------------------------------------------------------------===//
>
> +#include "lld/Driver/Driver.h"
>  #include "lld/Core/ArchiveLibraryFile.h"
>  #include "lld/Core/LLVM.h"
>  #include "lld/Core/LinkingContext.h"
> @@ -76,8 +77,11 @@
>      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.
> @@ -259,6 +263,7 @@
>    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 {
> Index: ReaderWriter/ELF/OutputELFWriter.cpp
> ===================================================================
> --- ReaderWriter/ELF/OutputELFWriter.cpp
> +++ ReaderWriter/ELF/OutputELFWriter.cpp
> @@ -45,8 +45,15 @@
>
>      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 atoms were added - return the file but also store it for later
> destruction
> +    if (_file->hasAtoms()) {
> +      File *result = _file.get();
> +      _returnedFiles.push_back(std::move(_file));
> +      return result;
> +    }
> +
> +    return nullptr;
>    }
>
>  private:
> @@ -57,6 +64,7 @@
>    // reversed destruction order.
>    llvm::BumpPtrAllocator _alloc;
>    unique_bump_ptr<SymbolFile<ELFT>> _file;
> +  std::vector<unique_bump_ptr<SymbolFile<ELFT>>> _returnedFiles;
>  };
>
>  } // end anon namespace
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150922/00f7d54e/attachment.html>