[PATCH 0/2] Fix some races seen by thread sanitizer

[PATCH 0/2] Fix some races seen by thread sanitizer

[Tom Tromey]

PR gdb/31264 points out some reports from thread sanitizer when
running some gdb tests.

I couldn't reproduce exactly these reports, but I did find that
bfd_check_format_matches uses some globals without locking, which is
definitely not thread-safe.  These are fixed in patch #1.

And, when working on this, I did manage to get other thread sanitizer
reports from the same gdb test case -- a problem when
bfd_check_format_matches is called simultaneously with
bfd_cache_close_all.  This is fixed in patch #2.

For patch #1, the per-xvec error-emission code was written to fix some
fuzzer bug, but I don't have access to the original test file here, so
I wasn't really able to test that the reimplementation is correct.

For patch #2, see the note in that commit about the archive case in
bfd_cache_lookup_worker.

Let me know what you think.

Tom

[PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Tom Tromey]

A gdb bug found that bfd_check_format_matches has some data races when
called from multiple threads.

In particular, it changes the BFD error handler, which is a global.
It also has a local static variable ("in_check_format") that is used
for recursion detection.  And, finally, it may emit warnings to the
per-xvec warning array, which is a global.

This patch removes all the races here.  I believe it does not change
the current semantics, but I was unable to get the test file that
would let me test that.

The first part of patch is to change _bfd_error_handler to directly
handle the needs of bfd_check_format_matches.  This way, the error
handler does not need to be changed.

This change lets us use the new per-thread global
(error_handler_messages, replacing error_handler_bfd) to also remove
the need for in_check_format -- a single variable suffices.

Finally, the global per-xvec array is replaced with a new type that
holds the error messages.  The outermost such type is stack-allocated
in bfd_check_format_matches.

I tested this using the binutils test suite.  I also built gdb with
thread sanitizer and ran the test case that was noted as failing.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
---
 bfd/bfd.c     | 68 ++++++++++++++++++++++++++++++++++++++-----------
 bfd/format.c  | 70 +++++++++++++++++++++++----------------------------
 bfd/libbfd.h  | 17 +++++++++++--
 bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
 4 files changed, 142 insertions(+), 72 deletions(-)

diff --git a/bfd/bfd.c b/bfd/bfd.c
index 54061a34240..11ad7f24456 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1534,10 +1534,17 @@ err_sprintf (void *stream, const char *fmt, ...)
   return total;
 }
 
-/* Communicate the bfd processed by bfd_check_format_matches to the
-   error handling function error_handler_sprintf.  */
+/* Communicate the error-message container processed by
+   bfd_check_format_matches to the error handling function
+   error_handler_sprintf.  When non-NULL, _bfd_error_handler will call
+   error_handler_sprintf; when NULL, _bfd_error_internal will be used
+   instead.  */
 
-static bfd *error_handler_bfd;
+static TLS struct per_xvec_messages *error_handler_messages;
+
+/* A special value for error_handler_messages that indicates that the
+   error should simply be ignored.  */
+#define IGNORE_ERROR_MESSAGES ((struct per_xvec_messages *) -1)
 
 /* An error handler that prints to a string, then dups that string to
    a per-xvec cache.  */
@@ -1554,12 +1561,12 @@ error_handler_sprintf (const char *fmt, va_list ap)
   _bfd_print (err_sprintf, &error_stream, fmt, ap);
 
   size_t len = error_stream.ptr - error_buf;
-  struct per_xvec_message **warn
-    = _bfd_per_xvec_warn (error_handler_bfd->xvec, len + 1);
-  if (*warn)
+  struct per_xvec_message *warn
+    = _bfd_per_xvec_warn (error_handler_messages, len + 1);
+  if (warn)
     {
-      memcpy ((*warn)->message, error_buf, len);
-      (*warn)->message[len] = 0;
+      memcpy (warn->message, error_buf, len);
+      warn->message[len] = 0;
     }
 }
 
@@ -1597,7 +1604,14 @@ _bfd_error_handler (const char *fmt, ...)
   va_list ap;
 
   va_start (ap, fmt);
-  _bfd_error_internal (fmt, ap);
+  if (error_handler_messages == IGNORE_ERROR_MESSAGES)
+    {
+      /* Nothing.  */
+    }
+  else if (error_handler_messages != NULL)
+    error_handler_sprintf (fmt, ap);
+  else
+    _bfd_error_internal (fmt, ap);
   va_end (ap);
 }
 
@@ -1628,18 +1642,42 @@ INTERNAL_FUNCTION
 	_bfd_set_error_handler_caching
 
 SYNOPSIS
-	bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
+	struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *);
 
 DESCRIPTION
 	Set the BFD error handler function to one that stores messages
-	to the per_xvec_warn array.  Returns the previous function.
+	to the per_xvec_warn array.  Returns the previous BFD to which
+	messages are stored.  Note that two sequential calls to this
+	with a non-NULL BFD will cause output to be dropped, rather
+	than gathered.
 */
 
-bfd_error_handler_type
-_bfd_set_error_handler_caching (bfd *abfd)
+struct per_xvec_messages *
+_bfd_set_error_handler_caching (struct per_xvec_messages *messages)
+{
+  struct per_xvec_messages *old = error_handler_messages;
+  if (old == NULL)
+    error_handler_messages = messages;
+  else
+    error_handler_messages = IGNORE_ERROR_MESSAGES;
+  return old;
+}
+
+/*
+INTERNAL_FUNCTION
+	_bfd_restore_error_handler_caching
+
+SYNOPSIS
+	void _bfd_restore_error_handler_caching (struct per_xvec_messages *);
+
+DESCRIPTION
+	Reset the BFD error handler function an earlier value.
+*/
+
+void
+_bfd_restore_error_handler_caching (struct per_xvec_messages *old)
 {
-  error_handler_bfd = abfd;
-  return bfd_set_error_handler (error_handler_sprintf);
+  error_handler_messages = old;
 }
 
 /*
diff --git a/bfd/format.c b/bfd/format.c
index 8f3fc7e7b96..5a5b01975ac 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -272,10 +272,33 @@ clear_warnmsg (struct per_xvec_message **list)
   *list = NULL;
 }
 
+/* Free all the storage in LIST.  Note that the first element of LIST
+   is special and is assumed to be stack-allocated.  TARG is used for
+   re-issuing warning messages.  If TARG is non-NULL, then any
+   messages in LIST with a matching target are issued.  If TARG is
+   NULL, then messages are only reissued if they are all associated
+   with a single BFD target.  */
+
 static void
-null_error_handler (const char *fmt ATTRIBUTE_UNUSED,
-		    va_list ap ATTRIBUTE_UNUSED)
+print_and_clear_messages (struct per_xvec_messages *list,
+			  const bfd_target *targ)
 {
+  struct per_xvec_messages *iter = list;
+
+  if (targ == NULL && list->next == NULL)
+    print_warnmsg (&list->messages);
+
+  while (iter != NULL)
+    {
+      struct per_xvec_messages *next = iter->next;
+
+      if (iter->targ == targ)
+	print_warnmsg (&iter->messages);
+      clear_warnmsg (&iter->messages);
+      if (iter != list)
+	free (iter);
+      iter = next;
+    }
 }
 
 /*
@@ -313,8 +336,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   unsigned int initial_section_id = _bfd_section_id;
   struct bfd_preserve preserve, preserve_match;
   bfd_cleanup cleanup = NULL;
-  bfd_error_handler_type orig_error_handler;
-  static int in_check_format;
+  struct per_xvec_messages messages = { abfd, NULL, NULL, NULL };
+  struct per_xvec_messages *orig_messages;
 
   if (matching != NULL)
     *matching = NULL;
@@ -345,11 +368,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 
   /* Don't report errors on recursive calls checking the first element
      of an archive.  */
-  if (in_check_format)
-    orig_error_handler = bfd_set_error_handler (null_error_handler);
-  else
-    orig_error_handler = _bfd_set_error_handler_caching (abfd);
-  ++in_check_format;
+  orig_messages = _bfd_set_error_handler_caching (&messages);
 
   preserve_match.marker = NULL;
   if (!bfd_preserve_save (abfd, &preserve, NULL))
@@ -591,15 +610,9 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       if (preserve_match.marker != NULL)
 	bfd_preserve_finish (abfd, &preserve_match);
       bfd_preserve_finish (abfd, &preserve);
-      bfd_set_error_handler (orig_error_handler);
+      _bfd_restore_error_handler_caching (orig_messages);
 
-      struct per_xvec_message **list = _bfd_per_xvec_warn (abfd->xvec, 0);
-      if (*list)
-	print_warnmsg (list);
-      list = _bfd_per_xvec_warn (NULL, 0);
-      for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-	clear_warnmsg (list++);
-      --in_check_format;
+      print_and_clear_messages (&messages, abfd->xvec);
 
       /* File position has moved, BTW.  */
       return true;
@@ -643,27 +656,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   if (preserve_match.marker != NULL)
     bfd_preserve_finish (abfd, &preserve_match);
   bfd_preserve_restore (abfd, &preserve);
-  bfd_set_error_handler (orig_error_handler);
-  struct per_xvec_message **list = _bfd_per_xvec_warn (NULL, 0);
-  struct per_xvec_message **one = NULL;
-  for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-    {
-      if (list[i])
-	{
-	  if (!one)
-	    one = list + i;
-	  else
-	    {
-	      one = NULL;
-	      break;
-	    }
-	}
-    }
-  if (one)
-    print_warnmsg (one);
-  for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-    clear_warnmsg (list++);
-  --in_check_format;
+  _bfd_restore_error_handler_caching (orig_messages);
+  print_and_clear_messages (&messages, NULL);
   return false;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index f15b5f27db8..04dbe720edb 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -934,7 +934,9 @@ void _bfd_clear_error_data (void) ATTRIBUTE_HIDDEN;
 
 char *bfd_asprintf (const char *fmt, ...) ATTRIBUTE_HIDDEN;
 
-bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
+struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
+
+void _bfd_restore_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
 
 const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;
 
@@ -3699,7 +3701,18 @@ struct per_xvec_message
   char message[];
 };
 
-struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t) ATTRIBUTE_HIDDEN;
+/* A list of per_xvec_message objects.  The targ field
+   indicates which xvec this list holds.  The abfd field is
+   only needed in the root entry of the list.  */
+struct per_xvec_messages
+{
+  bfd *abfd;
+  const bfd_target *targ;
+  struct per_xvec_message *messages;
+  struct per_xvec_messages *next;
+};
+
+struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t) ATTRIBUTE_HIDDEN;
 
 #ifdef __cplusplus
 }
diff --git a/bfd/targets.c b/bfd/targets.c
index f94992d031d..baf3cc5266c 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -1457,9 +1457,6 @@ const bfd_target *const *const bfd_associated_vector = _bfd_associated_vector;
    number of entries that the array could possibly need.  */
 const size_t _bfd_target_vector_entries = ARRAY_SIZE (_bfd_target_vector);
 
-/* A place to stash a warning from _bfd_check_format.  */
-static struct per_xvec_message *per_xvec_warn[ARRAY_SIZE (_bfd_target_vector)
-					      + 1];
 \f
 /* This array maps configuration triplets onto BFD vectors.  */
 
@@ -1488,11 +1485,22 @@ INTERNAL
 .  char message[];
 .};
 .
+.{* A list of per_xvec_message objects.  The targ field
+.   indicates which xvec this list holds.  The abfd field is
+.   only needed in the root entry of the list.  *}
+.struct per_xvec_messages
+.{
+.  bfd *abfd;
+.  const bfd_target *targ;
+.  struct per_xvec_message *messages;
+.  struct per_xvec_messages *next;
+.};
+.
 INTERNAL_FUNCTION
 	_bfd_per_xvec_warn
 
 SYNOPSIS
-	struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t);
+	struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t);
 
 DESCRIPTION
 	Return a location for the given target xvec to use for
@@ -1505,19 +1513,36 @@ DESCRIPTION
 	pointer to a NULL pointer on allocation failure.
 */
 
-struct per_xvec_message **
-_bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
+struct per_xvec_message *
+_bfd_per_xvec_warn (struct per_xvec_messages *messages, size_t alloc)
 {
-  size_t idx;
-
-  if (!targ)
-    return per_xvec_warn;
-  for (idx = 0; idx < ARRAY_SIZE (_bfd_target_vector); idx++)
-    if (_bfd_target_vector[idx] == targ)
-      break;
-  struct per_xvec_message **m = per_xvec_warn + idx;
-  if (!alloc)
-    return m;
+  const bfd_target *targ = messages->abfd->xvec;
+
+  struct per_xvec_messages *prev = NULL;
+  struct per_xvec_messages *iter = messages;
+
+  for (iter = messages; iter != NULL; iter = iter->next)
+    {
+      if (iter->targ == targ || iter->targ == NULL)
+	break;
+      prev = iter;
+    }
+
+  if (iter == NULL)
+    {
+      iter = bfd_malloc (sizeof (*iter));
+      if (iter == NULL)
+	return NULL;
+      iter->abfd = messages->abfd;
+      iter->targ = targ;
+      iter->messages = NULL;
+      iter->next = NULL;
+      prev->next = iter;
+    }
+  else if (iter->targ == NULL)
+    iter->targ = targ;
+
+  struct per_xvec_message **m = &iter->messages;
   int count = 0;
   while (*m)
     {
@@ -1531,7 +1556,7 @@ _bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
       if (*m != NULL)
 	(*m)->next = NULL;
     }
-  return m;
+  return *m;
 }
 
 /* Find a target vector, given a name or configuration triplet.  */
-- 
2.43.0

[PATCH 2/2] Avoid cache race in bfd_check_format_matches

[Tom Tromey]

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

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.  */
-- 
2.43.0

Re: [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This patch removes all the races here.  I believe it does not change
Tom> the current semantics, but I was unable to get the test file that
Tom> would let me test that.

Alan sent me the relevant file off-list, and I verified that the output
of 'nm-new' does not change after this series is applied.

I've locally updated the commit message for this patch to reflect that.

thanks,
Tom

Re: [PATCH 0/2] Fix some races seen by thread sanitizer

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> PR gdb/31264 points out some reports from thread sanitizer when
Tom> running some gdb tests.

Tom> I couldn't reproduce exactly these reports, but I did find that
Tom> bfd_check_format_matches uses some globals without locking, which is
Tom> definitely not thread-safe.  These are fixed in patch #1.

Tom> And, when working on this, I did manage to get other thread sanitizer
Tom> reports from the same gdb test case -- a problem when
Tom> bfd_check_format_matches is called simultaneously with
Tom> bfd_cache_close_all.  This is fixed in patch #2.

Tom> For patch #1, the per-xvec error-emission code was written to fix some
Tom> fuzzer bug, but I don't have access to the original test file here, so
Tom> I wasn't really able to test that the reimplementation is correct.

Tom> For patch #2, see the note in that commit about the archive case in
Tom> bfd_cache_lookup_worker.

Tom> Let me know what you think.

Ping.

These patches are on the short list blocking GDB 15, see:

https://sourceware.org/pipermail/gdb-patches/2024-April/207885.html

Tom

Re: [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Alan Modra]

On Sun, Mar 24, 2024 at 03:08:05PM -0600, Tom Tromey wrote:
> A gdb bug found that bfd_check_format_matches has some data races when
> called from multiple threads.
> 
> In particular, it changes the BFD error handler, which is a global.
> It also has a local static variable ("in_check_format") that is used
> for recursion detection.  And, finally, it may emit warnings to the
> per-xvec warning array, which is a global.
> 
> This patch removes all the races here.  I believe it does not change
> the current semantics, but I was unable to get the test file that
> would let me test that.

I suspect you have changed things a little.  The curent per_xvec_warn
array has an extra element to cache messages emitted when xvec is
NULL.  I added that just in case such a thing ever happened.  I don't
think it does, so I'm not at all concerned about a change in behaviour
when xvec is NULL.

However, the new _bfd_per_xvec_warn does seem to be a little weird
should messages->abfd-xvec ever be NULL.  It seems it will create a
new entry in the list for a NULL targ that then might be reused for a
later non-NULL targ message.  Also, the description for the new
_bfd_per_xvec_warn needs updating and maybe the function should be
made static and moved from targets.c to bfd.c.

> The first part of patch is to change _bfd_error_handler to directly
> handle the needs of bfd_check_format_matches.  This way, the error
> handler does not need to be changed.
> 
> This change lets us use the new per-thread global
> (error_handler_messages, replacing error_handler_bfd) to also remove
> the need for in_check_format -- a single variable suffices.
> 
> Finally, the global per-xvec array is replaced with a new type that
> holds the error messages.  The outermost such type is stack-allocated
> in bfd_check_format_matches.
> 
> I tested this using the binutils test suite.  I also built gdb with
> thread sanitizer and ran the test case that was noted as failing.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
> ---
>  bfd/bfd.c     | 68 ++++++++++++++++++++++++++++++++++++++-----------
>  bfd/format.c  | 70 +++++++++++++++++++++++----------------------------
>  bfd/libbfd.h  | 17 +++++++++++--
>  bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
>  4 files changed, 142 insertions(+), 72 deletions(-)
> 

>  SYNOPSIS
> -	bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
> +	struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *);
>  
>  DESCRIPTION
>  	Set the BFD error handler function to one that stores messages
> -	to the per_xvec_warn array.  Returns the previous function.
> +	to the per_xvec_warn array.  Returns the previous BFD to which
> +	messages are stored.  Note that two sequential calls to this
> +	with a non-NULL BFD will cause output to be dropped, rather
> +	than gathered.
>  */

non-NULL BFD?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 2/2] Avoid cache race in bfd_check_format_matches

[Alan Modra]

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.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 0/2] Fix some races seen by thread sanitizer

[Nick Clifton]

Hi Tom,

> PR gdb/31264 points out some reports from thread sanitizer when
> running some gdb tests.
> 
> I couldn't reproduce exactly these reports, but I did find that
> bfd_check_format_matches uses some globals without locking, which is
> definitely not thread-safe.  These are fixed in patch #1.
> 
> And, when working on this, I did manage to get other thread sanitizer
> reports from the same gdb test case -- a problem when
> bfd_check_format_matches is called simultaneously with
> bfd_cache_close_all.  This is fixed in patch #2.
> 
> For patch #1, the per-xvec error-emission code was written to fix some
> fuzzer bug, but I don't have access to the original test file here, so
> I wasn't really able to test that the reimplementation is correct.
> 
> For patch #2, see the note in that commit about the archive case in
> bfd_cache_lookup_worker.
> 
> Let me know what you think.

Personally I do not think that any library function should ever call
abort(), but I guess that in this case the coder wanted to be made
aware immediately if the function was ever called with an entirely
artifical bfd (ie one just held in memory with no on-disk version).

Anyway, patch series approved, please apply.

Cheers
   Nick

Re: [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Tom Tromey]

>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:

Alan> I suspect you have changed things a little.  The curent per_xvec_warn
Alan> array has an extra element to cache messages emitted when xvec is
Alan> NULL.  I added that just in case such a thing ever happened.  I don't
Alan> think it does, so I'm not at all concerned about a change in behaviour
Alan> when xvec is NULL.

Alan> However, the new _bfd_per_xvec_warn does seem to be a little weird
Alan> should messages->abfd-xvec ever be NULL.  It seems it will create a
Alan> new entry in the list for a NULL targ that then might be reused for a
Alan> later non-NULL targ message.  Also, the description for the new
Alan> _bfd_per_xvec_warn needs updating and maybe the function should be
Alan> made static and moved from targets.c to bfd.c.

Thanks for the review.

I've made these changes, and since I was already in there I went ahead
and fixed the NULL xvec issue as well.

I'll send v2 momentarily.

Tom