[PATCH v2] gdb: fix crash when GDB can't read an objfile
Guinevere Larsen
guinevere@redhat.com
Tue Dec 3 14:43:06 GMT 2024
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.
>
> Thanks,
> Andrew
>
>
>
>> Reported-By: Andrew Burgess <aburgess@redhat.com>
>> ---
>> gdb/compile/compile-object-load.c | 6 +++---
>> gdb/objfiles.h | 10 ++++++++--
>> gdb/symfile.c | 9 ++++++---
>> 3 files changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
>> index df48b1c54f5..8b1556e8f0a 100644
>> --- a/gdb/compile/compile-object-load.c
>> +++ b/gdb/compile/compile-object-load.c
>> @@ -641,9 +641,9 @@ compile_object_load (const compile_file_names &file_names,
>>
>> /* SYMFILE_VERBOSE is not passed even if FROM_TTY, user is not interested in
>> "Reading symbols from ..." message for automatically generated file. */
>> - objfile_up objfile_holder (symbol_file_add_from_bfd (abfd,
>> - filename.get (),
>> - 0, NULL, 0, NULL));
>> + scoped_objfile_unlinker objfile_holder (symbol_file_add_from_bfd
>> + (abfd, filename.get (),
>> + 0, NULL, 0, NULL));
>> objfile = objfile_holder.get ();
>>
>> func_sym = lookup_global_symbol_from_objfile (objfile,
>> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
>> index d92570a00bd..4f555274ec8 100644
>> --- a/gdb/objfiles.h
>> +++ b/gdb/objfiles.h
>> @@ -630,6 +630,9 @@ struct objfile
>> gdb_assert (section->owner == nullptr || section->owner == this->obfd);
>>
>> int idx = gdb_bfd_section_index (this->obfd.get (), section);
>> +
>> + /* Guarantee that the section offsets were initialized. */
>> + gdb_assert (this->section_offsets.size () > idx);
>> return this->section_offsets[idx];
>> }
>>
>> @@ -641,6 +644,9 @@ struct objfile
>> gdb_assert (section->owner == nullptr || section->owner == this->obfd);
>>
>> int idx = gdb_bfd_section_index (this->obfd.get (), section);
>> +
>> + /* Guarantee that the section offsets were initialized. */
>> + gdb_assert (this->section_offsets.capacity () > idx);
>> this->section_offsets[idx] = offset;
>> }
>>
>> @@ -887,7 +893,7 @@ struct objfile
>>
>> /* A deleter for objfile. */
>>
>> -struct objfile_deleter
>> +struct objfile_unlinker
>> {
>> void operator() (objfile *ptr) const
>> {
>> @@ -897,7 +903,7 @@ struct objfile_deleter
>>
>> /* A unique pointer that holds an objfile. */
>>
>> -typedef std::unique_ptr<objfile, objfile_deleter> objfile_up;
>> +typedef std::unique_ptr<objfile, objfile_unlinker> scoped_objfile_unlinker;
>>
>> /* Relocation offset applied to the section. */
>> inline CORE_ADDR
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index 1502fdbe500..4e9411aff3e 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -886,6 +886,11 @@ syms_from_objfile_1 (struct objfile *objfile,
>> section_addr_info local_addr;
>> const int mainline = add_flags & SYMFILE_MAINLINE;
>>
>> + /* If we can't find a sym_fns struct to read the objfile, we'll error
>> + out, and should unlink the objfile from the program space. So this
>> + should be declared before a find_sym_fns call. */
>> + scoped_objfile_unlinker objfile_holder (objfile);
>> +
>> objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd.get ()));
>> objfile->qf.clear ();
>>
>> @@ -903,8 +908,6 @@ syms_from_objfile_1 (struct objfile *objfile,
>> if an error occurs during symbol reading. */
>> std::optional<clear_symtab_users_cleanup> defer_clear_users;
>>
>> - objfile_up objfile_holder (objfile);
>> -
>> /* If ADDRS is NULL, put together a dummy address list.
>> We now establish the convention that an addr of zero means
>> no load address was specified. */
>> @@ -2522,7 +2525,7 @@ reread_symbols (int from_tty)
>> /* If we get an error, blow away this objfile (not sure if
>> that is the correct response for things like shared
>> libraries). */
>> - objfile_up objfile_holder (objfile);
>> + scoped_objfile_unlinker objfile_holder (objfile);
>>
>> /* We need to do this whenever any symbols go away. */
>> clear_symtab_users_cleanup defer_clear_users (0);
>> --
>> 2.47.0
--
Cheers,
Guinevere Larsen
She/Her/Hers
More information about the Gdb-patches
mailing list