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

Wed Dec 4 16:56:11 GMT 2024

On 12/4/24 1:24 PM, Simon Marchi wrote:
>
> 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
>
Oh whoops. Thanks for finding this, I'll take a look at it ASAP.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers