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

[Pedro Alves <pedro@palves.net>]

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.