Re: [PATCH] Call _close_and_cleanup in bfd_close_all_done

[Alan Modra <amodra at gmail dot com>]

On Wed, Aug 30, 2017 at 05:46:14AM -0700, H.J. Lu wrote:
> On Tue, Aug 29, 2017 at 11:49 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Tue, Aug 29, 2017 at 12:05:31PM -0700, H.J. Lu wrote:
> >> OK for master?
> >>
> >> H.J.
> >> ---
> >>       PR binutils/22032
> >>       * opncls.c (bfd_close_all_done): Call _close_and_cleanup.
> >
> > Please have a look at archive.c:_bfd_archive_close_and_cleanup and
> > convince yourself that this change is safe.  archive_close_worker
> > calls bfd_close_all_done.  I think it is OK, but I'd like to make sure
> > you have put eyes on that code too.
> >
[snip]
> Since an archive member can never be the archive which it is in,
> it is safe.

Yeah, I thought it was good too, but --enable-targets=all objdump -i
now dies with a segfault.  HJ, testing only x86 is just not good
enough.

The problem appears to be due to elf64_vms_close_and_cleanup calling
bfd_get_size, which calls iovec->bstat.  cache_bstat ends up adding
the bfd to the cache lru list, negating the bfd_cache_close call in
bfd_close_all_done.  So there is a dangling pointer into the freed and
then reused bfd.

I'm testing the following fix.

	* opncls.c (bfd_close_all_done): Don't call bfd_cache_close
	before _close_and_cleanup.  Call iovec->bclose after.
	(bfd_close): Remove code common to, and call bfd_close_all_done.

diff --git a/bfd/opncls.c b/bfd/opncls.c
index b99ae72..fa54986 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -726,25 +726,13 @@ RETURNS
 bfd_boolean
 bfd_close (bfd *abfd)
 {
-  bfd_boolean ret;
-
   if (bfd_write_p (abfd))
     {
       if (! BFD_SEND_FMT (abfd, _bfd_write_contents, (abfd)))
 	return FALSE;
     }
 
-  if (! BFD_SEND (abfd, _close_and_cleanup, (abfd)))
-    return FALSE;
-
-  ret = abfd->iovec->bclose (abfd) == 0;
-
-  if (ret)
-    _maybe_make_executable (abfd);
-
-  _bfd_delete_bfd (abfd);
-
-  return ret;
+  return bfd_close_all_done (abfd);
 }
 
 /*
@@ -774,11 +762,11 @@ bfd_close_all_done (bfd *abfd)
 {
   bfd_boolean ret;
 
-  ret = bfd_cache_close (abfd);
-
   if (! BFD_SEND (abfd, _close_and_cleanup, (abfd)))
     return FALSE;
 
+  ret = abfd->iovec->bclose (abfd) == 0;
+
   if (ret)
     _maybe_make_executable (abfd);
 

-- 
Alan Modra
Australia Development Lab, IBM