Fix races involving _bfd_section_id
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
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
---
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(-)
@@ -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
@@ -12675,6 +12675,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);
@@ -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;
}
@@ -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;
@@ -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;
}