Fix races involving _bfd_section_id

BFD's threading approach is that global variables are guarded by a
lock.  However, while implementing this, I missed _bfd_section_id.  A
user pointed out, via Thread Sanitizier, that this causes a data race
when gdb's background DWARF reader is enabled.

This patch fixes the problem by using the BFD lock in most of the
appropriate spots.  However, in ppc64_elf_setup_section_lists I chose
to simply assert that multiple threads are not in use instead.  (Not
totally sure if this is good, but I don't think this can be called by
gdb.)

I chose locking in bfd_check_format_matches, even though it is a
relatively big hammer, because it seemed like the most principled
approach, and anyway if this causes severe contention we can always
revisit the decision.  Also this approach means we don't need to add
configury to check for _Atomic, or figure out whether bfd_section_init
can be reworded to make "rollback" unnecessary.

I couldn't reproduce these data races but the original reporter tested
the patch and confirms that it helps.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713

5 files changed, 43 insertions, 2 deletions

diff --git a/bfd/bfd.c b/bfd/bfd.c
index a93be10..1eef9ec 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1957,6 +1957,23 @@ static bfd_lock_unlock_fn_type unlock_fn;
 static void *lock_data;
 
 /*
+INTERNAL_FUNCTION
+	_bfd_threading_enabled
+
+SYNOPSIS
+	bool _bfd_threading_enabled (void);
+
+DESCRIPTION
+	Return true if threading is enabled, false if not.
+*/
+
+bool
+_bfd_threading_enabled (void)
+{
+  return lock_fn != NULL;
+}
+
+/*
 FUNCTION
 	bfd_thread_init
 
@@ -1974,7 +1991,9 @@ DESCRIPTION
 	success and false on error.  DATA is passed verbatim to the
 	lock and unlock functions.  The lock and unlock functions
 	should return true on success, or set the BFD error and return
-	false on failure.
+	false on failure.  Note also that the lock must be a recursive
+	lock: BFD may attempt to acquire the lock when it is already
+	held by the current thread.
 */
 
 bool
diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index f7debc1..dd1ee1e 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -12684,6 +12684,10 @@ ppc64_elf_setup_section_lists (struct bfd_link_info *info)
   if (htab == NULL)
     return -1;
 
+  /* The access to _bfd_section_id here is unlocked, so for the time
+     being this function cannot be called in multi-threaded mode.  */
+  BFD_ASSERT (!_bfd_threading_enabled ());
+
   htab->sec_info_arr_size = _bfd_section_id;
   amt = sizeof (*htab->sec_info) * (htab->sec_info_arr_size);
   htab->sec_info = bfd_zmalloc (amt);
diff --git a/bfd/format.c b/bfd/format.c
index d0af388..3736ee9 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -451,6 +451,10 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
      of an archive.  */
   orig_messages = _bfd_set_error_handler_caching (&messages);
 
+  /* Locking is required here in order to manage _bfd_section_id.  */
+  if (!bfd_lock ())
+    return false;
+
   preserve_match.marker = NULL;
   if (!bfd_preserve_save (abfd, &preserve, NULL))
     goto err_ret;
@@ -698,7 +702,10 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       bfd_set_lto_type (abfd);
 
       /* File position has moved, BTW.  */
-      return bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
+      bool ret = bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
+      if (!bfd_unlock ())
+	return false;
+      return ret;
     }
 
   if (match_count == 0)
@@ -742,6 +749,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   _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);
+  bfd_unlock ();
   return false;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 7d7ae1e..d4be334 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -993,6 +993,8 @@ void _bfd_restore_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_H
 
 const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;
 
+bool _bfd_threading_enabled (void) ATTRIBUTE_HIDDEN;
+
 bool bfd_lock (void) ATTRIBUTE_HIDDEN;
 
 bool bfd_unlock (void) ATTRIBUTE_HIDDEN;
diff --git a/bfd/section.c b/bfd/section.c
index 81def03..35e4489 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -839,6 +839,10 @@ unsigned int _bfd_section_id = 0x10;  /* id 0 to 3 used by STD_SECTION.  */
 static asection *
 bfd_section_init (bfd *abfd, asection *newsect)
 {
+  /* Locking needed for the _bfd_section_id access.  */
+  if (!bfd_lock ())
+    return NULL;
+
   newsect->id = _bfd_section_id;
   newsect->index = abfd->section_count;
   newsect->owner = abfd;
@@ -849,6 +853,10 @@ bfd_section_init (bfd *abfd, asection *newsect)
   _bfd_section_id++;
   abfd->section_count++;
   bfd_section_list_append (abfd, newsect);
+
+  if (!bfd_unlock ())
+    return NULL;
+
   return newsect;
 }