bfd_check_format_matches preserving matches vs. cleanups

It didn't take long for oss-fuzz to find double frees due to a bug in
the cleanup logic.  It's seen when reading in any alpha-vms object
file except when alpha_vms_vec is the default.  But alpha_vms_vec is
of course the default when building for --target=alpha-dec-vms (and
naturally what I used to  test the cleanup support since that is the
only target with a cleanup that does anything currently).

Anyway, the bug is that if bfd_check_format_matches is to preserve a
match the cleanup for that match can't be run.  Quite obviously that
would destroy part of the match state.

	* format.c (struct bfd_preserve): Add cleanup field.
	(bfd_preserve_save): Add cleanup param and save.
	(bfd_preserve_restore): Return cleanup.
	(bfd_preserve_finish): Call the cleanup for the discarded match.
	(bfd_check_format_matches): Pass cleanup to bfd_preserve_save,
	and clear when preserving a match.  Restore cleanup too when
	restoring that match.

2 files changed, 29 insertions, 5 deletions

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 1f9e205..ff2881f 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,13 @@
+2020-03-03  Alan Modra  <amodra@gmail.com>
+
+	* format.c (struct bfd_preserve): Add cleanup field.
+	(bfd_preserve_save): Add cleanup param and save.
+	(bfd_preserve_restore): Return cleanup.
+	(bfd_preserve_finish): Call the cleanup for the discarded match.
+	(bfd_check_format_matches): Pass cleanup to bfd_preserve_save,
+	and clear when preserving a match.  Restore cleanup too when
+	restoring that match.
+
 2020-03-02  Alan Modra  <amodra@gmail.com>
 
 	* cisco-core.c (cisco_core_file_p): Return bfd_cleanup.
diff --git a/bfd/format.c b/bfd/format.c
index b181742..61f26c6 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -106,6 +106,7 @@ struct bfd_preserve
   unsigned int section_id;
   struct bfd_hash_table section_htab;
   const struct bfd_build_id *build_id;
+  bfd_cleanup cleanup;
 };
 
 /* When testing an object for compatibility with a particular target
@@ -118,7 +119,8 @@ struct bfd_preserve
    the subset.  */
 
 static bfd_boolean
-bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve)
+bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
+		   bfd_cleanup cleanup)
 {
   preserve->tdata = abfd->tdata.any;
   preserve->arch_info = abfd->arch_info;
@@ -130,6 +132,7 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve)
   preserve->section_htab = abfd->section_htab;
   preserve->marker = bfd_alloc (abfd, 1);
   preserve->build_id = abfd->build_id;
+  preserve->cleanup = cleanup;
   if (preserve->marker == NULL)
     return FALSE;
 
@@ -153,7 +156,7 @@ bfd_reinit (bfd *abfd, unsigned int section_id, bfd_cleanup cleanup)
 
 /* Restores bfd state saved by bfd_preserve_save.  */
 
-static void
+static bfd_cleanup
 bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
 {
   bfd_hash_table_free (&abfd->section_htab);
@@ -172,6 +175,7 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
      its arg, as well as its arg.  */
   bfd_release (abfd, preserve->marker);
   preserve->marker = NULL;
+  return preserve->cleanup;
 }
 
 /* Called when the bfd state saved by bfd_preserve_save is no longer
@@ -180,6 +184,15 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
 static void
 bfd_preserve_finish (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_preserve *preserve)
 {
+  if (preserve->cleanup)
+    {
+      /* Run the cleanup, assuming that all it will need is the
+	 tdata at the time the cleanup was returned.  */
+      void *tdata = abfd->tdata.any;
+      abfd->tdata.any = preserve->tdata;
+      preserve->cleanup (abfd);
+      abfd->tdata.any = tdata;
+    }
   /* It would be nice to be able to free more memory here, eg. old
      tdata, but that's not possible since these blocks are sitting
      inside bfd_alloc'd memory.  The section hash is on a separate
@@ -252,7 +265,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   save_targ = abfd->xvec;
 
   preserve_match.marker = NULL;
-  if (!bfd_preserve_save (abfd, &preserve))
+  if (!bfd_preserve_save (abfd, &preserve, NULL))
     goto err_ret;
 
   /* If the target type was explicitly specified, just check that target.  */
@@ -381,8 +394,9 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 	  if (preserve_match.marker == NULL)
 	    {
 	      match_targ = abfd->xvec;
-	      if (!bfd_preserve_save (abfd, &preserve_match))
+	      if (!bfd_preserve_save (abfd, &preserve_match, cleanup))
 		goto err_ret;
+	      cleanup = NULL;
 	    }
 	}
     }
@@ -455,7 +469,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
      whole bfd and restoring it would be even worse; the first thing
      you notice is that the cached bfd file position gets out of sync.  */
   if (preserve_match.marker != NULL)
-    bfd_preserve_restore (abfd, &preserve_match);
+    cleanup = bfd_preserve_restore (abfd, &preserve_match);
 
   if (match_count == 1)
     {