[PATCH v2] gdb: fix crash when GDB can't read an objfile

[Simon Marchi]

On 2024-12-03 09:43, Guinevere Larsen wrote:
> On 12/3/24 10:09 AM, Andrew Burgess wrote:
>> Guinevere Larsen <guinevere@redhat.com> writes:
>>
>>> If a user starts an inferior composed of objfiles that GDB is unable to
>>> read, there is an error thrown in find_sym_fns, printing the famous "I'm
>>> sorry, Dave, I can't do that" and the objfile stops being read. However,
>>> the objfile will already have been linked to the program space, and
>>> future interactions with the objfile will assume that it is readable.
>>>
>>> Relevant to this commit, if GDB tries to find out the section that
>>> contains a PC, and this section happens to land in the unreadable
>>> objfile, GDB will try to create a section mapping, eventually calling
>>> update_section_map. Since that function uses bfd to calculate the
>>> sections, it'll think there are sections to be ordered, but when trying
>>> to access the objfile::section_offsets, it'll be indexing a size 0
>>> std::vector, which will end up segfaulting.
>>>
>>> Currently, it isn't easy to trigger this crash, but the upcoming
>>> possibility to disable support for some file formats would make the
>>> crash very easy to reproduce, by attempting to debug an unsupported
>>> inferior and using "break *<instruction>" command, or simply connecting
>>> to a gdbserver loaded with an unsupported inferior.
>>>
>>> The struct objfile_up seems to have been created to catch these kinds of
>>> errors and unlink the partially-read objfile from the program space, as
>>> the objfile isn't useful to GDB anymore, but it seems to have been added
>>> before find_sym_fns would throw errors for unreadable objfiles, as the
>>> instance in syms_from_objfile_1 (that could save GDB from this crash) is
>>> declared well after find_sym_fns, too late to guard us. This commit
>>> moves the declaration up to the top of the function, so it works as
>>> intended.
>>>
>>> Further discussion on the mailing list also agreed that the name
>>> "objfile_up" implies some level of ownership of the pointer, which this
>>> struct doesn't have. So this commit renames the struct to
>>> scoped_objfile_unlinker, which is more descriptive of what the struct is
>>> actually meant to do.
>>>
>>> The final change this commit does is add an assertion to
>>> objfile::section_offset and objfile::set_section_offset, which ensures
>>> that the section_offsets vector is large enough to return the desired
>>> offset. This ensures that we won't misteriously segfault or worse,
>>> continue going with garbage data.
>> I'm going to approve this patch, even though it appears that Tom's
>> request here:
>>
>>    https://inbox.sourceware.org/gdb-patches/877c9m6f9b.fsf@tromey.com
>>
>> hasn't been addressed.  I think the change Tom is suggesting would
>> potentially be an improvement, but it is, I think, clearly a bigger
>> change that what is presented here.
>>
>> If we ignore the name change included in this patch, then all that's
>> done here is move the RAII object creation earlier within
>> syms_from_objfile_1, and add a couple of asserts.  The name change makes
>> sense given the object isn't really acting as a unique_ptr.
>>
>> If/when Tom's suggestion for a new model of objfile ownership comes
>> along, implementing that on top of this patch will be no harder (I
>> think), so merging this patch now, and getting the benefits it brings
>> seems sensible.
>>
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
> thanks for the review! Pushed.


Hi Guinevere,

Since this commit, I see this when running gdb.base/dump.exp:

(gdb) file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dump/intarr1.srec
Load new symbol table from "/home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dump/intarr1.srec"? (y or n) y
Reading symbols from /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dump/intarr1.srec...
=================================================================
==2290398==ERROR: AddressSanitizer: heap-use-after-free on address 0x51600006bdc0 at pc 0x642590ff684c bp 0x7fff7d180b90 sp 0x7fff7d180b80
READ of size 8 at 0x51600006bdc0 thread T0
    #0 0x642590ff684b in init_entry_point_info /home/simark/src/binutils-gdb/gdb/symfile.c:790
    #1 0x642590ff88a4 in syms_from_objfile /home/simark/src/binutils-gdb/gdb/symfile.c:975
    #2 0x642590ff99ae in symbol_file_add_with_addrs /home/simark/src/binutils-gdb/gdb/symfile.c:1079
    #3 0x642590ffa7d5 in symbol_file_add_from_bfd(gdb::ref_ptr<bfd, gdb_bfd_ref_policy> const&, char const*, enum_flags<symfile_add_flag>, std::__debug::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>, objfile*) /home/simark/src/binutils-gdb/gdb/symfile.c:1153
    #4 0x642590ffa951 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, std::__debug::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>) /home/simark/src/binutils-gdb/gdb/symfile.c:1166
    #5 0x642590ffad0f in symbol_file_add_main_1 /home/simark/src/binutils-gdb/gdb/symfile.c:1190
    #6 0x642590ffea35 in symbol_file_command(char const*, int) /home/simark/src/binutils-gdb/gdb/symfile.c:1620
    #7 0x64258fc968d1 in file_command /home/simark/src/binutils-gdb/gdb/exec.c:566
    #8 0x64258f11aaca in do_simple_func /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:94
    #9 0x64258f131064 in cmd_func(cmd_list_element*, char const*, int) /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:2741
    #10 0x64259138ae1d in execute_command(char const*, int) /home/simark/src/binutils-gdb/gdb/top.c:570
    #11 0x64258fc85679 in command_handler(char const*) /home/simark/src/binutils-gdb/gdb/event-top.c:613
    #12 0x64258fc86a13 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/simark/src/binutils-gdb/gdb/event-top.c:849
    #13 0x6425914e85fe in tui_command_line_handler /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:101
    #14 0x64258fc835fb in gdb_rl_callback_handler /home/simark/src/binutils-gdb/gdb/event-top.c:288

Simon