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

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

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