[PATCH 0/2] Fix a data race with the background DWARF reader

[PATCH 0/2] Fix a data race with the background DWARF reader

[Tom Tromey]

This series fixes a race in the background DWARF reader.

I was able to reproduce the original race manually (though not via
dejagnu), and then confirmed (at least, there weren't warnings from
TSAN) that this series fixes the problem.

---
Tom Tromey (2):
      Use gdb_bfd_get_full_section_contents in auto-load.c
      Add locking when reading BFD sections

 gdb/auto-load.c | 11 ++++-------
 gdb/gdb_bfd.c   | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 7 deletions(-)
---
base-commit: 876f0e78ba8244aa5705530fd8522928f05e6783
change-id: 20241017-fix-race-pr-symtab-31626-e88148cf55a2

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/2] Use gdb_bfd_get_full_section_contents in auto-load.c

[Tom Tromey]

This changes auto-load.c ot use gdb_bfd_get_full_section_contents.
This shouldn't change any behavior, but makes it easier to add locking
in a subsequent patch.
---
 gdb/auto-load.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index e753333b1cdfe6a12b1885d1468ef92ef51776af..d4307eac6273803ed7024491169701e8a55fcc6b 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -1114,25 +1114,22 @@ auto_load_section_scripts (struct objfile *objfile, const char *section_name)
 {
   bfd *abfd = objfile->obfd.get ();
   asection *scripts_sect;
-  bfd_byte *data = NULL;
 
   scripts_sect = bfd_get_section_by_name (abfd, section_name);
   if (scripts_sect == NULL
       || (bfd_section_flags (scripts_sect) & SEC_HAS_CONTENTS) == 0)
     return;
 
-  if (!bfd_get_full_section_contents (abfd, scripts_sect, &data))
+  gdb::byte_vector data;
+  if (!gdb_bfd_get_full_section_contents (abfd, scripts_sect, &data))
     warning (_("Couldn't read %s section of %ps"),
 	     section_name,
 	     styled_string (file_name_style.style (),
 			    bfd_get_filename (abfd)));
   else
     {
-      gdb::unique_xmalloc_ptr<bfd_byte> data_holder (data);
-
-      char *p = (char *) data;
-      source_section_scripts (objfile, section_name, p,
-			      p + bfd_section_size (scripts_sect));
+      const char *p = (const char *) data.data ();
+      source_section_scripts (objfile, section_name, p, p + data.size ());
     }
 }
 

-- 
2.46.1

[PATCH 2/2] Add locking when reading BFD sections

[Tom Tromey]

This adds some per-BFD locking to gdb_bfd_map_section and
gdb_bfd_get_full_section_contents.

It turned out that the background DWARF reader could race with the
auto-load code, because the reader might try to mmap a section when
the main thread was trying to read in .debug_gdb_scripts.

The current BFD threading model is that only BFD globals will be
locked, so any multi-threaded use of a BFD has to be handled specially
by the application.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31626
---
 gdb/gdb_bfd.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index b142f985dcd99e6d29f4a6b295ebff50ca851293..fff71d0c3bdb900e180ec136429bf66c030fd1bc 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -144,6 +144,19 @@ struct gdb_bfd_data
 
   /* The registry.  */
   registry<bfd> registry_fields;
+
+#if CXX_STD_THREAD
+  /* Most of the locking needed for multi-threaded operation is
+     handled by BFD itself.  However, the current BFD model is that
+     locking is only needed for global operations -- but it turned out
+     that the background DWARF reader could race with the auto-load
+     code reading the .debug_gdb_scripts section from the same BFD.
+
+     This lock is the fix: wrappers for important BFD functions will
+     acquire this lock before performing operations that might modify
+     the state of this BFD.  */
+  std::mutex per_bfd_mutex;
+#endif
 };
 
 registry<bfd> *
@@ -777,6 +790,11 @@ gdb_bfd_map_section (asection *sectp, bfd_size_type *size)
 
   abfd = sectp->owner;
 
+#if CXX_STD_THREAD
+  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
+  std::lock_guard<std::mutex> guard (gdata->per_bfd_mutex);
+#endif
+
   descriptor = get_section_descriptor (sectp);
 
   /* If the data was already read for this BFD, just reuse it.  */
@@ -1108,6 +1126,11 @@ bool
 gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
 				   gdb::byte_vector *contents)
 {
+#if CXX_STD_THREAD
+  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
+  std::lock_guard<std::mutex> guard (gdata->per_bfd_mutex);
+#endif
+
   bfd_size_type section_size = bfd_section_size (section);
 
   contents->resize (section_size);

-- 
2.46.1

Re: [PATCH 0/2] Fix a data race with the background DWARF reader

[Kevin Buettner]

On Thu, 17 Oct 2024 18:02:36 -0600
Tom Tromey <tom@tromey.com> wrote:

> This series fixes a race in the background DWARF reader.
> 
> I was able to reproduce the original race manually (though not via
> dejagnu), and then confirmed (at least, there weren't warnings from
> TSAN) that this series fixes the problem.
> 
> ---
> Tom Tromey (2):
>       Use gdb_bfd_get_full_section_contents in auto-load.c
>       Add locking when reading BFD sections
> 
>  gdb/auto-load.c | 11 ++++-------
>  gdb/gdb_bfd.c   | 23 +++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 7 deletions(-)
> ---
> base-commit: 876f0e78ba8244aa5705530fd8522928f05e6783
> change-id: 20241017-fix-race-pr-symtab-31626-e88148cf55a2
> 
> Best regards,
> -- 
> Tom Tromey <tom@tromey.com>
> 

Both patches LGTM.

Reviewed-by: Kevin Buettner <kevinb@redhat.com>