From: Guinevere Larsen Date: Mon, 21 Oct 2024 18:57:55 +0000 (-0300) Subject: gdb: fix crash when GDB can't read an objfile X-Git-Tag: gdb-16-branchpoint~208 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=32e3f1a0aa0aec359d944da029ea7b7262d259cd;p=thirdparty%2Fbinutils-gdb.git gdb: fix crash when GDB can't read an objfile 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 *" 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. Reported-By: Andrew Burgess Approved-By: Andrew Burgess --- 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 2121fd31792..3e4b5727ee9 100644 --- a/gdb/objfiles.h +++ b/gdb/objfiles.h @@ -631,6 +631,9 @@ public: 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]; } @@ -642,6 +645,9 @@ public: 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; } @@ -889,7 +895,7 @@ public: /* A deleter for objfile. */ -struct objfile_deleter +struct objfile_unlinker { void operator() (objfile *ptr) const { @@ -899,7 +905,7 @@ struct objfile_deleter /* A unique pointer that holds an objfile. */ -typedef std::unique_ptr objfile_up; +typedef std::unique_ptr scoped_objfile_unlinker; /* Relocation offset applied to the section. */ inline CORE_ADDR diff --git a/gdb/symfile.c b/gdb/symfile.c index 4bc0c4916bd..3fd6c8d73a2 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 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);