[lld] r221544 - Fix FileArchive member MemoryBuffer early destruction

[David Blaikie]

On Fri, Nov 7, 2014 at 12:52 PM, Nick Kledzik <kledzik at apple.com> wrote:

> Author: kledzik
> Date: Fri Nov  7 14:52:38 2014
> New Revision: 221544
>
> URL: http://llvm.org/viewvc/llvm-project?rev=221544&view=rev
> Log:
> Fix FileArchive member MemoryBuffer early destruction
>
> When FileArchive loads a member, it instantiates a temporary MemoryBuffer
> which points to the member range of the archive file.  The problem is that
> the
> object file parsers call getBufferIndentifer() on that temporary
> MemoryBuffer
> and store that StringRef as the _path data member for that lld::File.  When
> FileArchive::instantiateMember() goes out of scope the MemoryBuffer is
> deleted
> and the File::._path becomes a dangling reference.
>
> The fix adds a vector<> to FileArchive to own the instantiated
> MemoryBuffers.
> In addition it fixes member's path to be the standard format
> (e.g. "/path/libfoo.a(foo.o)") instead of just the leaf name.
>
> Modified:
>     lld/trunk/lib/ReaderWriter/FileArchive.cpp
>
> Modified: lld/trunk/lib/ReaderWriter/FileArchive.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/FileArchive.cpp?rev=221544&r1=221543&r2=221544&view=diff
>
> ==============================================================================
> --- lld/trunk/lib/ReaderWriter/FileArchive.cpp (original)
> +++ lld/trunk/lib/ReaderWriter/FileArchive.cpp Fri Nov  7 14:52:38 2014
> @@ -133,18 +133,27 @@ private:
>      if (std::error_code ec = mbOrErr.getError())
>        return ec;
>      llvm::MemoryBufferRef mb = mbOrErr.get();
> +    std::string memberPath = (_archive->getFileName() + "("
> +                           + mb.getBufferIdentifier() + ")").str();
> +
>      if (_logLoading)
> -      llvm::outs() << _archive->getFileName() << "(" <<
> mb.getBufferIdentifier()
> -                   << ")"
> -                   << "\n";
> +      llvm::errs() << memberPath << "\n";
>
> -    std::unique_ptr<MemoryBuffer> buf(MemoryBuffer::getMemBuffer(
> -        mb.getBuffer(), mb.getBufferIdentifier(), false));
> +    std::unique_ptr<MemoryBuffer> memberMB(MemoryBuffer::getMemBuffer(
> +        mb.getBuffer(), memberPath, false));
>

It's somewhat handy to use copy/move initialization rather than direct
initialization where possible. ie favor:

std::unique_ptr<MemoryBuffer> memberMB = MemoryBuffer::getMemBuffer(...);

over "memberMB(MemoryBuffer::getMemBuffer..." in this case.

That way it's clear that the function returns ownership via unique_ptr, not
raw pointer - and there's no risk of an explicit conversion from raw
pointer to unique_ptr occurring accidentally (while in this case it's
pretty clear/known that getMemBuffer returns ownership, writing it with
move initialization makes that more explicit).


>
>      std::vector<std::unique_ptr<File>> files;
> -    _registry.parseFile(buf, files);
> +    _registry.parseFile(memberMB, files);
>      assert(files.size() == 1);
>      result = std::move(files[0]);
> +
> +    // Note: The object file parsers use getBufferIdentifier() from
> memberMB
> +    // for the file path. And MemoryBuffer makes its own copy of the path.
> +    // That means when if memberMB is destroyed, the lld:File objects will
> +    // have a dangling reference for their path.  To fix that, all the
> +    // MemoryBuffers for the archive members are owned by _memberBuffers.
> +    _memberBuffers.push_back(std::move(memberMB));
> +
>      return std::error_code();
>    }
>
> @@ -205,6 +214,7 @@ private:
>    atom_collection_vector<AbsoluteAtom> _absoluteAtoms;
>    bool _isWholeArchive;
>    bool _logLoading;
> +  mutable std::vector<std::unique_ptr<MemoryBuffer>> _memberBuffers;
>  };
>
>  class ArchiveReader : public Reader {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141111/a11e0c60/attachment.html>