[binutils-gdb] Avoid cache race in bfd_check_format_matches

[binutils-gdb] Avoid cache race in bfd_check_format_matches

[Tom Tromey]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=20bf7711bce2188c1af985a87df23c4653c2cc7a

commit 20bf7711bce2188c1af985a87df23c4653c2cc7a
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Mar 23 15:19:20 2024 -0600

    Avoid cache race in bfd_check_format_matches
    
    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

Diff:
---
 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(-)

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 109de767a39..e3b5a8b8522 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -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;
diff --git a/bfd/bfd.c b/bfd/bfd.c
index ace2f67954f..ae79c6490b5 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -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;
diff --git a/bfd/cache.c b/bfd/cache.c
index 0f994c74239..5c825433b62 100644
--- a/bfd/cache.c
+++ b/bfd/cache.c
@@ -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
diff --git a/bfd/format.c b/bfd/format.c
index 2a700bab557..443fc6dbde0 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -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;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index d1062620d65..5e8ed9eeefe 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -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.  */