[binutils-gdb] Fix races involving _bfd_section_id

[Tom Tromey]

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

commit e1093de6a0feeb44a6a2f3bb4509d35eb28bcb66
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Nov 1 17:19:20 2024 -0600

    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

Diff:
---
 bfd/bfd.c       | 21 ++++++++++++++++++++-
 bfd/elf64-ppc.c |  4 ++++
 bfd/format.c    | 10 +++++++++-
 bfd/libbfd.h    |  2 ++
 bfd/section.c   |  8 ++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/bfd/bfd.c b/bfd/bfd.c
index a93be109f11..1eef9ecdacc 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1956,6 +1956,23 @@ static bfd_lock_unlock_fn_type lock_fn;
 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 f7debc1edbc..dd1ee1eb9ea 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 d0af388ab45..3736ee9c802 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 7d7ae1eaec9..d4be334bf87 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 81def037e6a..35e4489c475 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;
 }