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.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 1f9e20581fe1194f52a4ef4bd28401611d5a6739..ff2881f966fe4444ed4febd3f543ddad75220203 100644 (file)
--- 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 b181742f3b1699660b4e493f60f81bc6dc7ca757..61f26c6042ae3b3f957951c793cae12d1b1e3811 100644 (file)
--- 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)
     {