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 --git a/bfd/bfd.c b/bfd/bfd.c
index a93be109f11135d44637e81fb92a7e57a2930cd3..1eef9ecdacce90a9c47071bb94a003729a0919b0 100644 (file)
--- 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 f7debc1edbc46e593292e3c89800c14f6f228fd3..dd1ee1eb9ea95548a105069339be130feea2f35a 100644 (file)
--- 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 d0af388ab45f864bf5d728ea0a26ef26cf1fb43b..3736ee9c8025a79ca0efcd407a9f0dcb8f26496b 100644 (file)
--- 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 7d7ae1eaec9c7e9d38a56142495669d630db712b..d4be334bf87d0a31225dfc34646e1386e2118db8 100644 (file)
--- 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 81def037e6ad37926624fbec5129e7375626ad6c..35e4489c47562bab76909d1aeda1c07f03a19c12 100644 (file)
--- 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;
 }