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

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

[Tom de Vries]

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;
   }
 

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

[Pedro Alves]

On 2022-07-12 9:00 a.m., Tom de Vries via Gdb-patches wrote:
> 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?

Not sure about this, it seems a bit too hacky to me.  Doesn't this mean that gdb_bfd_section_index
ends up returning the same index for two different sections?  Like, in your example above, it returns 6
for both the new COMMON section added by bfd_simple_get_relocated_section_contents and *ABS*?

If the count of bfd sections can grow behind our backs, couldn't we solve the index problem
by giving sections *ABS*, *UND*, *COM* and *IND* indexes 0 through 3, and then the
non-absolute bfd sections would start at 4 ?  I.e., there would be a bias
of 4 between gdb section numbers and bfd section numbers, but maybe that wouldn't
be a real problem?  This way, if bfd sections grow, the preexisting
absolute section indexes would remain stable.

Also, don't we end up with the objfile->sections array with one section too few?  Like, won't it
be missing a slot for the new COMMON bfd section?  Are we growing that array somewhere after
default_symfile_relocate is called?

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

[Tom de Vries]

On 7/12/22 11:30, Pedro Alves wrote:
> On 2022-07-12 9:00 a.m., Tom de Vries via Gdb-patches wrote:
>> 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?
> 
> Not sure about this, it seems a bit too hacky to me. 

I agree, it's far from ideal, and its only merit seems to me that it 
improves upon the current situation.

> Doesn't this mean that gdb_bfd_section_index
> ends up returning the same index for two different sections? > Like, in your example above, it returns 6
> for both the new COMMON section added by bfd_simple_get_relocated_section_contents and *ABS*?
> 

That's not the case.

So, we have count == 6, as per:
...
   int count = get_orig_bfd_count_sections (abfd);
...

For *ABS*, it returns 8, as per:
...
   else if (section == bfd_abs_section_ptr)
     return count + 2; 
               ....

Perhaps you mean *COM*, for which it returns 6:
...
   else if (section == bfd_com_section_ptr)
     return count; 
               ...

Anyway, for COMMON, with bfd section index 6, it asserts:
...
+  gdb_assert (section->index < count); 
               ...

> If the count of bfd sections can grow behind our backs, couldn't we solve the index problem
> by giving sections *ABS*, *UND*, *COM* and *IND* indexes 0 through 3, and then the
> non-absolute bfd sections would start at 4 ?  I.e., there would be a bias
> of 4 between gdb section numbers and bfd section numbers, but maybe that wouldn't
> be a real problem?  This way, if bfd sections grow, the preexisting
> absolute section indexes would remain stable.
> 

Yes, I tried that, I didn't get that to work.  I suppose it'll require 
using gdb_bfd_section_index in a lot more places.  And where to use it 
and where not is not easy to see if both the bfd section index and the 
gdb_bfd section index are the same type.  I've also tried making those 
different types without implicit conversion, but also didn't manage to 
drive that to completion.

> Also, don't we end up with the objfile->sections array with one section too few?  Like, won't it
> be missing a slot for the new COMMON bfd section?  Are we growing that array somewhere after
> default_symfile_relocate is called?

AFAIU, neither the sections and sections_offsets array are grown.  I've 
also looked into fixing that but am not familiar enough with the code to 
understand what to put in the sections_offset array.

Thanks,
- Tom

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

[Pedro Alves]

On 2022-07-12 11:16 a.m., Tom de Vries wrote:
> On 7/12/22 11:30, Pedro Alves wrote:
>> On 2022-07-12 9:00 a.m., Tom de Vries via Gdb-patches wrote:
>>> 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?
>>
>> Not sure about this, it seems a bit too hacky to me. 
> 
> I agree, it's far from ideal, and its only merit seems to me that it improves upon the current situation.
> 
>> Doesn't this mean that gdb_bfd_section_index
>> ends up returning the same index for two different sections? > Like, in your example above, it returns 6
>> for both the new COMMON section added by bfd_simple_get_relocated_section_contents and *ABS*?
>>
> 
> That's not the case.
> 
> So, we have count == 6, as per:
> ...
>   int count = get_orig_bfd_count_sections (abfd);
> ...
> 
> For *ABS*, it returns 8, as per:
> ...
>   else if (section == bfd_abs_section_ptr)
>     return count + 2;               ....
> 
> Perhaps you mean *COM*, for which it returns 6:
> ...
>   else if (section == bfd_com_section_ptr)
>     return count;               ...
> 
> Anyway, for COMMON, with bfd section index 6, it asserts:
> ...
> +  gdb_assert (section->index < count);               ...
> 
>> If the count of bfd sections can grow behind our backs, couldn't we solve the index problem
>> by giving sections *ABS*, *UND*, *COM* and *IND* indexes 0 through 3, and then the
>> non-absolute bfd sections would start at 4 ?  I.e., there would be a bias
>> of 4 between gdb section numbers and bfd section numbers, but maybe that wouldn't
>> be a real problem?  This way, if bfd sections grow, the preexisting
>> absolute section indexes would remain stable.
>>
> 
> Yes, I tried that, I didn't get that to work.  I suppose it'll require using gdb_bfd_section_index in a lot more places.  And where to use it and where not is not easy to see if both the bfd section index and the gdb_bfd section index are the same type.  I've also tried making those different types without implicit conversion, but also didn't manage to drive that to completion.
> 
>> Also, don't we end up with the objfile->sections array with one section too few?  Like, won't it
>> be missing a slot for the new COMMON bfd section?  Are we growing that array somewhere after
>> default_symfile_relocate is called?
> 
> AFAIU, neither the sections and sections_offsets array are grown.  I've also looked into fixing that but am not familiar enough with the code to understand what to put in the sections_offset array.

Another question is, why do the bfd sections grow in the first place?  Maybe that's a bfd bug?  Like, why
isn't COMMON already in the bfd sections list when the bfd is first opened?  Maybe that could be an angle
to tackle this.

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

[Tom de Vries]

On 7/12/22 12:25, Pedro Alves wrote:
> On 2022-07-12 11:16 a.m., Tom de Vries wrote:
>> On 7/12/22 11:30, Pedro Alves wrote:
>>> On 2022-07-12 9:00 a.m., Tom de Vries via Gdb-patches wrote:
>>>> 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?
>>>
>>> Not sure about this, it seems a bit too hacky to me.
>>
>> I agree, it's far from ideal, and its only merit seems to me that it improves upon the current situation.
>>
>>> Doesn't this mean that gdb_bfd_section_index
>>> ends up returning the same index for two different sections? > Like, in your example above, it returns 6
>>> for both the new COMMON section added by bfd_simple_get_relocated_section_contents and *ABS*?
>>>
>>
>> That's not the case.
>>
>> So, we have count == 6, as per:
>> ...
>>    int count = get_orig_bfd_count_sections (abfd);
>> ...
>>
>> For *ABS*, it returns 8, as per:
>> ...
>>    else if (section == bfd_abs_section_ptr)
>>      return count + 2;               ....
>>
>> Perhaps you mean *COM*, for which it returns 6:
>> ...
>>    else if (section == bfd_com_section_ptr)
>>      return count;               ...
>>
>> Anyway, for COMMON, with bfd section index 6, it asserts:
>> ...
>> +  gdb_assert (section->index < count);               ...
>>
>>> If the count of bfd sections can grow behind our backs, couldn't we solve the index problem
>>> by giving sections *ABS*, *UND*, *COM* and *IND* indexes 0 through 3, and then the
>>> non-absolute bfd sections would start at 4 ?  I.e., there would be a bias
>>> of 4 between gdb section numbers and bfd section numbers, but maybe that wouldn't
>>> be a real problem?  This way, if bfd sections grow, the preexisting
>>> absolute section indexes would remain stable.
>>>
>>
>> Yes, I tried that, I didn't get that to work.  I suppose it'll require using gdb_bfd_section_index in a lot more places.  And where to use it and where not is not easy to see if both the bfd section index and the gdb_bfd section index are the same type.  I've also tried making those different types without implicit conversion, but also didn't manage to drive that to completion.
>>
>>> Also, don't we end up with the objfile->sections array with one section too few?  Like, won't it
>>> be missing a slot for the new COMMON bfd section?  Are we growing that array somewhere after
>>> default_symfile_relocate is called?
>>
>> AFAIU, neither the sections and sections_offsets array are grown.  I've also looked into fixing that but am not familiar enough with the code to understand what to put in the sections_offset array.
> 
> Another question is, why do the bfd sections grow in the first place?  Maybe that's a bfd bug?  Like, why
> isn't COMMON already in the bfd sections list when the bfd is first opened?  Maybe that could be an angle
> to tackle this.

Yes, that is a possibility. I see in bfd/MAINTAINERS that the binutils 
maintainers maintain it, so we could ask, perhaps at bug-binutils.

Thanks,
- Tom

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Another question is, why do the bfd sections grow in the first
Pedro> place?

It does seem bad if this can happen randomly.  If it can be done
intentionally though we could have gdb take whatever action is needed
when first reading the BFD to get the full section count.

All this stuff with section indices only exists for the case where an
objfile is loaded using different offsets for different sections.  Can
this really happen (aside from absolute sections)?  If not maybe we
could just get rid of all of it.

Tom

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

[Pedro Alves]

On 2022-07-15 7:55 p.m., Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Another question is, why do the bfd sections grow in the first
> Pedro> place?
> 
> It does seem bad if this can happen randomly.  If it can be done
> intentionally though we could have gdb take whatever action is needed
> when first reading the BFD to get the full section count.

See Alan's reply here:

 https://lists.gnu.org/archive/html/bug-binutils/2022-07/msg00128.html

His last sentence:

 "Hmm, I suppose you could argue that since this is done for the linker, there is no
 need to do so for simple_get_relocated_section_contents."

... suggests this could be fixed in bfd.  I have no idea what that entails, but if it's
just not doing something, then maybe it's simple enough?

> 
> All this stuff with section indices only exists for the case where an
> objfile is loaded using different offsets for different sections.  Can
> this really happen (aside from absolute sections)?  If not maybe we
> could just get rid of all of it.

Given:

  add-symbol-file FILE [-s SECT-NAME SECT-ADDR]...

... I think that yes, it happens.

Also, there are targets where shared libraries aren't really fully
linked libraries, but instead relocatable objects (https://sourceware.org/gdb/current/onlinedocs/gdb/Library-List-Format.html),
and for those, I don't think we can assume a single offset works for all sections.

Even if we have fully linked binaries and segments, then it can be that different
segments are loaded independently, with different bias offsets?  That would mean different
offsets for different sections too.