who owns a BFD coming from an archive?

who owns a BFD coming from an archive?

[Tom Tromey]

As part of a gdb patch series I've been looking into how BFD treats
archive members.

Suppose you have an archive BFD, opened with bfd_openr with a subsequent
successful call to bfd_check_format (archive, bfd_archive).  Now suppose
you iterate over the members of the archive using
bfd_openr_next_archived_file.

In this case, my experiments yield some weird results:

* For an ordinary archive, according to valgrind (and my reading of the
  code, though I found it a bit hard to follow...), the caller is
  expected to close each member BFD using bfd_close.  If you don't call
  bfd_close on each one, you get memory leaks.

  However, there is a latent bug here.  If the member is closed, it does
  not notify the parent BFD, and so the archive cache (e.g.,
  archive.c:_bfd_look_for_bfd_in_cache) refers to freed memory.

  Also, the memory for the members' file names is allocated in memory
  attached to the parent archive.  So, if you close the parent archive,
  bfd_get_filename on a member will yield a freed pointer.

* For a thin archive, again according to valgrind and my reading, the
  opposite is true: the archive owns the BFDs, and if you close each
  one, you will see crashes.

* Finally, nothing ever frees the hash table created by
  _bfd_add_bfd_to_archive_cache.


My first question is -- is my analysis correct?
I think I got it all right, but perhaps I am missing something.

My second question is, anybody have a definitive opinion on the best way
to fix the problems?

I think it would be best for clients if both ordinary and thin archives
were treated in the same way.

For gdb's purposes I think it would be a little more convenient if an
archive and its members were independent, but I can make gdb work with
either ownership approach.

The cache poisoning and deletion problems seem relatively
straightforward to fix, though I'm probably not really conversant enough
with BFD to pick the right spot to hack.

For the filename problem, in gdb we recently adopted a rule to
(re-)allocate a BFD's file name using bfd_alloc.  This ties the lifetime
of the filename to that of the BFD, usually what you want.  So, one idea
here would be to promote this idea into BFD; but again, I don't really
care much, I would just like some guidance from you.

I'm happy to implement the desired fixes.

thanks,
Tom

Re: who owns a BFD coming from an archive?

[Alan Modra]

On Mon, Jul 23, 2012 at 02:11:33PM -0600, Tom Tromey wrote:
> * For a thin archive, again according to valgrind and my reading, the
>   opposite is true: the archive owns the BFDs, and if you close each
>   one, you will see crashes.

Thin archives support nesting.  I think the BFDs opened to access
nested archives are owned by the archive, BFDs returned for objects
are owned by you.  However it is all quite nasty due to the archive
cache.  Some binutils and ld don't bother with closing archive member
BFDs, trusting that when the process exits all sins will be forgiven.
See for example this comment in ld/plugin.c
      /* BFD archive handling caches elements so we can't call
	 bfd_close for archives.  */

I think you'll see crashes due to the cache with normal archives too,
eg. call get_elt_at_index, close the returned bfd, then call
get_elt_at_index with the same position again.  You can also easily
crash with openr_next_archived_file if you close the current member
bfd before opening the next one..

-- 
Alan Modra
Australia Development Lab, IBM

Re: who owns a BFD coming from an archive?

[Tom Tromey]

>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:

Tom> * For a thin archive, again according to valgrind and my reading, the
Tom> opposite is true: the archive owns the BFDs, and if you close each
Tom> one, you will see crashes.

Alan> Thin archives support nesting.  I think the BFDs opened to access
Alan> nested archives are owned by the archive, BFDs returned for objects
Alan> are owned by you.  However it is all quite nasty due to the archive
Alan> cache.

Thanks for replying.

I wrote some patches to fix all the problems I know of.
They fix my simple test case, but I'm still looking through the rest of
binutils, etc, to make sure they will work ok (or to fix up whatever
issues I see).

I found out that I was confused about thin archives.  What I was seeing
is that with flattened thin archives, the outer archive BFD owns the
inner archive BFD -- but this inner archive BFD is invisible to the BFD
user, so the special treatment inside BFD is warranted.

Alan> I think you'll see crashes due to the cache with normal archives too,
Alan> eg. call get_elt_at_index, close the returned bfd, then call
Alan> get_elt_at_index with the same position again.  You can also easily
Alan> crash with openr_next_archived_file if you close the current member
Alan> bfd before opening the next one..

Yeah.  I didn't change this, on the theory that it is implied by the API
for bfd_openr_next_archived_file.


The approach I took is:

* Archive members must be closed like any other BFD.
  (You can of course still leave them open and exit.)
* However, archive members aren't fully independent of their parent
  BFD.  So, if you close the parent, you must not use any open children.

I looked at trying to make archive members fully independent, but this
looked difficult.

Maybe it could be done for thin archive members, but I thought
consistency was preferable.

I didn't deal with output BFD archives and handling archive_head.  I
don't know anything about those.


If this sounds like a bad plan, I'd appreciate a heads up.

Tom