[PATCH][gdb/symtab] Fix out-of-bounds in objfile::section_offset

[Tom de Vries <tdevries@suse.de>]

Hi,

Using this patch in objfile::section_offset that checks that idx is within
bounds:
...
     int idx = gdb_bfd_section_index (this->obfd, section);
+    gdb_assert (idx < section_offsets.size ());
     return this->section_offsets[idx];
...
we run into the assert in test-cases:
- gdb.base/readline-ask.exp
- gdb.base/symbol-without-target_section.exp
- gdb.dwarf2/dw2-icc-opaque.exp

These were previously reported as -fsanitize-threads issues (PR25724,
PR25723).

In the case of the latter test-case the problem happens as follows.

- We start out with bfd_count_sections () == 6, so
  gdb_bfd_count_sections () == 10.  The difference of 4 is due to the
  4 'special sections' named *ABS*, *UND*, *COM* and *IND*.
- According to gdb_bfd_section_index, the *IND* has section index
  bfd_count_sections () + 3, so 9.
- default_symfile_relocate gets called, which calls
  bfd_simple_get_relocated_section_contents and eventually this results in
  bfd_make_section_old_way being called for a section named COMMON,
  meaning now we have bfd_count_sections () == 7
- consequently the next time we call objfile::section_offset for *IND*,
  gdb_bfd_section_index assigns it section index 10.
- the assert fails because idx == 10 and section_offsets.size () == 10.

Fix this in a minimal and contained way, by:
- adding a side-table orig_bfd_count_sections_map, in which we store the
  original bfd_count_sections () value, and
- using this value in gdb_bfd_count_sections and gdb_bfd_section_index,
  ensuring that the creation of the new section doesn't interfere with
  accessing the unchanged objfile::sections and objfile::section_offsets.

In case we call gdb_bfd_section_index with the new section, we assert.

However, in case gdb_bfd_section_index is not used, and the bfd section index
of the new section is used to access objfile::sections or
objfile::section_offsets, we return some unrelated element, which might fail
in some difficult to understand manner.  It's hard to check whether this can
happen or not without having distinct types for the different section indices
(bfd vs. gdb_bfd).  Anyway, if this does occur, it's a pre-existing bug.  This
patch focuses on getting things right for the original sections.

Tested on x86_64-linux, with and without -fsanitize=threads.

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

Any comments?

Thanks,
- Tom

[gdb/symtab] Fix out-of-bounds in objfile::section_offset

---
 gdb/gdb_bfd.c  | 32 +++++++++++++++++++++++++++-----
 gdb/objfiles.h |  2 ++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 22828482d5b..66b6ad1c0de 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -34,6 +34,10 @@
 #include "inferior.h"
 #include "cli/cli-style.h"
 
+/* The quantity bfd_count_sections can change over time.  Store
+   the original value here.  */
+static std::unordered_map<bfd *, int> orig_bfd_count_sections_map;
+
 /* An object of this type is stored in the section's user data when
    mapping a section.  */
 
@@ -730,6 +734,7 @@ gdb_bfd_unref (struct bfd *abfd)
   bfd_set_usrdata (abfd, NULL);  /* Paranoia.  */
 
   htab_remove_elt (all_bfds, abfd);
+  orig_bfd_count_sections_map.erase (abfd);
 
   gdb_bfd_close_or_warn (abfd);
 
@@ -996,21 +1001,37 @@ gdb_bfd_record_inclusion (bfd *includer, bfd *includee)
 
 gdb_static_assert (ARRAY_SIZE (_bfd_std_section) == 4);
 
+/* The quantity bfd_count_sections can change over time.  Return
+   the original value here.  */
+
+static int
+get_orig_bfd_count_sections (bfd *abfd)
+{
+  auto it = orig_bfd_count_sections_map.find (abfd);
+  if (it != orig_bfd_count_sections_map.end ())
+    return it->second;
+  int idx = bfd_count_sections (abfd);
+  orig_bfd_count_sections_map[abfd] = idx;
+  return idx;
+}
+
 /* See gdb_bfd.h.  */
 
 int
 gdb_bfd_section_index (bfd *abfd, asection *section)
 {
+  int count = get_orig_bfd_count_sections (abfd);
   if (section == NULL)
     return -1;
   else if (section == bfd_com_section_ptr)
-    return bfd_count_sections (abfd);
+    return count;
   else if (section == bfd_und_section_ptr)
-    return bfd_count_sections (abfd) + 1;
+    return count + 1;
   else if (section == bfd_abs_section_ptr)
-    return bfd_count_sections (abfd) + 2;
+    return count + 2;
   else if (section == bfd_ind_section_ptr)
-    return bfd_count_sections (abfd) + 3;
+    return count + 3;
+  gdb_assert (section->index < count);
   return section->index;
 }
 
@@ -1019,7 +1040,8 @@ gdb_bfd_section_index (bfd *abfd, asection *section)
 int
 gdb_bfd_count_sections (bfd *abfd)
 {
-  return bfd_count_sections (abfd) + 4;
+  int count = get_orig_bfd_count_sections (abfd);
+  return count + 4;
 }
 
 /* See gdb_bfd.h.  */
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index a7098b46279..faaf97feb1f 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -598,6 +598,7 @@ struct objfile
     gdb_assert (section->owner == nullptr || section->owner == this->obfd);
 
     int idx = gdb_bfd_section_index (this->obfd, section);
+    gdb_assert (idx != -1);
     return this->section_offsets[idx];
   }
 
@@ -609,6 +610,7 @@ struct objfile
     gdb_assert (section->owner == nullptr || section->owner == this->obfd);
 
     int idx = gdb_bfd_section_index (this->obfd, section);
+    gdb_assert (idx != -1);
     this->section_offsets[idx] = offset;
   }