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

[Pedro Alves <pedro@palves.net>]

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?