From: Alan Modra <amodra@gmail.com>
To: binutils@sourceware.org
Subject: bfd_check_format_matches preserving matches vs. cleanups
Date: Tue, 03 Mar 2020 11:14:00 -0000 [thread overview]
Message-ID: <20200303111417.GN5384@bubble.grove.modra.org> (raw)
In-Reply-To: <20200302085904.GH5384@bubble.grove.modra.org>
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/format.c b/bfd/format.c
index b181742f3b..61f26c6042 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)
{
--
Alan Modra
Australia Development Lab, IBM
prev parent reply other threads:[~2020-03-03 11:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 8:59 bfd_cleanup for object_p Alan Modra
2020-03-02 12:38 ` H.J. Lu
2020-03-02 13:22 ` Alan Modra
2020-03-02 13:46 ` Alan Modra
2020-03-03 11:14 ` Alan Modra [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200303111417.GN5384@bubble.grove.modra.org \
--to=amodra@gmail.com \
--cc=binutils@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).