Running the gdb test suite with the thread sanitizer enabled shows a
race when bfd_check_format_matches and bfd_cache_close_all are called
simultaneously on different threads.
This patch fixes this race by having bfd_check_format_matches
temporarily remove the BFD from the file descriptor cache -- leaving
it open while format-checking proceeds.
In this setup, the BFD client is responsible for closing the BFD again
on the "checking" thread, should that be desired. gdb does this by
calling bfd_cache_close in the relevant worker thread.
An earlier version of this patch omitted the "possibly_cached" helper
function. However, this ran into crashes in the binutils test suite
involving the archive-checking abort in bfd_cache_lookup_worker. I do
not understand the purpose of this check, so I've simply had the new
function work around it. I couldn't find any comments explaining this
situation, either. I suspect that there may still be races related to
this case, but I don't think I have access to the platforms where gdb
deals with archives.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
---
bfd/bfd-in2.h | 6 ++++
bfd/bfd.c | 6 ++++
bfd/cache.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++---
bfd/format.c | 16 +++++++++-
bfd/libbfd.h | 2 ++
5 files changed, 109 insertions(+), 5 deletions(-)
@@ -2186,6 +2186,12 @@ struct bfd
/* LTO object type. */
ENUM_BITFIELD (bfd_lto_object_type) lto_type : 2;
+ /* Set if this BFD is currently being processed by
+ bfd_check_format_matches. This is checked by the cache to
+ avoid closing the BFD in this case. This should only be
+ examined or modified while the BFD lock is held. */
+ unsigned int in_format_matches : 1;
+
/* Set to dummy BFD created when claimed by a compiler plug-in
library. */
bfd *plugin_dummy_bfd;
@@ -307,6 +307,12 @@ CODE_FRAGMENT
. {* LTO object type. *}
. ENUM_BITFIELD (bfd_lto_object_type) lto_type : 2;
.
+. {* Set if this BFD is currently being processed by
+. bfd_check_format_matches. This is checked by the cache to
+. avoid closing the BFD in this case. This should only be
+. examined or modified while the BFD lock is held. *}
+. unsigned int in_format_matches : 1;
+.
. {* Set to dummy BFD created when claimed by a compiler plug-in
. library. *}
. bfd *plugin_dummy_bfd;
@@ -226,6 +226,20 @@ close_one (void)
? (FILE *) (bfd_last_cache->iostream) \
: bfd_cache_lookup_worker (x, flag))
+/* A helper function that returns true if ABFD can possibly be cached
+ -- that is, whether bfd_cache_lookup_worker will accept it. */
+
+static bool
+possibly_cached (bfd *abfd)
+{
+ if ((abfd->flags & BFD_IN_MEMORY) != 0)
+ return false;
+ if (abfd->my_archive != NULL
+ && !bfd_is_thin_archive (abfd->my_archive))
+ return false;
+ return true;
+}
+
/* Called when the macro <<bfd_cache_lookup>> fails to find a
quick answer. Find a file descriptor for @var{abfd}. If
necessary, it open it. If there are already more than
@@ -236,12 +250,17 @@ close_one (void)
static FILE *
bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
{
- if ((abfd->flags & BFD_IN_MEMORY) != 0)
+ if (!possibly_cached (abfd))
abort ();
- if (abfd->my_archive != NULL
- && !bfd_is_thin_archive (abfd->my_archive))
- abort ();
+ /* If the BFD is being processed by bfd_check_format_matches, it
+ must already be open and won't be on the list. */
+ if (abfd->in_format_matches)
+ {
+ if (abfd->iostream == NULL)
+ abort ();
+ return (FILE *) abfd->iostream;
+ }
if (abfd->iostream != NULL)
{
@@ -654,6 +673,63 @@ bfd_cache_close_all (void)
return ret;
}
+/*
+INTERNAL_FUNCTION
+ bfd_cache_set_uncloseable
+
+SYNOPSIS
+ bool bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old);
+
+DESCRIPTION
+ Internal function to mark ABFD as either closeable or not.
+ This is used by bfd_check_format_matches to avoid races
+ where bfd_cache_close_all is called in another thread.
+ VALUE is true to mark the BFD as temporarily uncloseable
+ by the cache; false to mark it as closeable once again.
+ OLD, if non-NULL, is set to the previous value of the flag.
+ Returns false on error, true on success.
+*/
+
+bool
+bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old)
+{
+ bool result = true;
+
+ if (!bfd_lock ())
+ return false;
+ if (old != NULL)
+ *old = abfd->in_format_matches;
+
+ /* Only perform any action when the state changes,and only when this
+ BFD is actually using the cache. */
+ if (value != abfd->in_format_matches
+ && abfd->iovec == &cache_iovec
+ && possibly_cached (abfd))
+ {
+ if (value)
+ {
+ /* Marking as uncloseable for the first time. Ensure the
+ file is open, and remove from the cache list. */
+ FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL);
+ if (f == NULL)
+ result = false;
+ else
+ snip (abfd);
+ }
+ else
+ {
+ /* Mark as closeable again. */
+ insert (abfd);
+ }
+
+ abfd->in_format_matches = value;
+ }
+
+ if (!bfd_unlock ())
+ return false;
+ return result;
+}
+
/*
FUNCTION
bfd_cache_size
@@ -86,6 +86,13 @@ DESCRIPTION
o <<bfd_error_file_ambiguously_recognized>> -
more than one backend recognised the file format.
+
+ When calling bfd_check_format (or bfd_check_format_matches),
+ any underlying file descriptor will be kept open for the
+ duration of the call. This is done to avoid races when
+ another thread calls bfd_cache_close_all. In this scenario,
+ the thread calling bfd_check_format must call bfd_cache_close
+ itself.
*/
bool
@@ -383,6 +390,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
bfd_cleanup cleanup = NULL;
struct per_xvec_messages messages = { abfd, PER_XVEC_NO_TARGET, NULL, NULL };
struct per_xvec_messages *orig_messages;
+ bool old_in_format_matches;
if (matching != NULL)
*matching = NULL;
@@ -410,6 +418,11 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
return false;
}
+ /* Avoid clashes with bfd_cache_close_all running in another
+ thread. */
+ if (!bfd_cache_set_uncloseable (abfd, true, &old_in_format_matches))
+ return false;
+
/* Presume the answer is yes. */
abfd->format = format;
save_targ = abfd->xvec;
@@ -665,7 +678,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
bfd_set_lto_type (abfd);
/* File position has moved, BTW. */
- return true;
+ return bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
}
if (match_count == 0)
@@ -708,6 +721,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
bfd_preserve_restore (abfd, &preserve);
_bfd_restore_error_handler_caching (orig_messages);
print_and_clear_messages (&messages, PER_XVEC_NO_TARGET);
+ bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
return false;
}
@@ -1055,6 +1055,8 @@ void *bfd_arch_default_fill (bfd_size_type count,
/* Extracted from cache.c. */
bool bfd_cache_init (bfd *abfd) ATTRIBUTE_HIDDEN;
+bool bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old) ATTRIBUTE_HIDDEN;
+
FILE* bfd_open_file (bfd *abfd) ATTRIBUTE_HIDDEN;
/* Extracted from hash.c. */