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

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

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