[2/2] Avoid cache race in bfd_check_format_matches

Message ID 20240324211229.1444550-3-tom@tromey.com
State New
Headers
Series Fix some races seen by thread sanitizer |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Tom Tromey March 24, 2024, 9:08 p.m. UTC
  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(-)
  

Comments

Alan Modra April 10, 2024, 3:46 a.m. UTC | #1
On Sun, Mar 24, 2024 at 03:08:06PM -0600, Tom Tromey wrote:
> 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.

This all looks OK.
  

Patch

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index fa28688837c..ceb53683d21 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2160,6 +2160,12 @@  struct bfd
      that BFD is not prepared to handle for objcopy/strip.  */
   unsigned int read_only : 1;
 
+  /* 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 11ad7f24456..ca4250327d2 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -285,6 +285,12 @@  CODE_FRAGMENT
 .     that BFD is not prepared to handle for objcopy/strip.  *}
 .  unsigned int read_only : 1;
 .
+.  {* 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 d0e7be293a5..c526dcae09f 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)
     {
@@ -657,6 +676,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 5a5b01975ac..238a76f14b9 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
@@ -338,6 +345,7 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   bfd_cleanup cleanup = NULL;
   struct per_xvec_messages messages = { abfd, NULL, NULL, NULL };
   struct per_xvec_messages *orig_messages;
+  bool old_in_format_matches;
 
   if (matching != NULL)
     *matching = NULL;
@@ -362,6 +370,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;
@@ -615,7 +628,7 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       print_and_clear_messages (&messages, abfd->xvec);
 
       /* File position has moved, BTW.  */
-      return true;
+      return bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
     }
 
   if (match_count == 0)
@@ -658,6 +671,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, NULL);
+  bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
   return false;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 04dbe720edb..5863a658f8a 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -1017,6 +1017,8 @@  bfd_window_internal;
 /* 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.  */