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 *<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.

Reported-By: Andrew Burgess <aburgess@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index df48b1c54f5b393fbce9682f4124112a28706e85..8b1556e8f0af08bb8a733facb517abb9d5779ce8 100644 (file)
--- 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 2121fd31792e861a99289249acd6a977c2a22a7a..3e4b5727ee96bc75552f35ed4dffe74ac4c2dde9 100644 (file)
--- 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, 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 4bc0c4916bde4018e591dfee4a547976316012f3..3fd6c8d73a2bc2050dbe5a40a71990c8255e15d1 100644 (file)
--- 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);