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>

3 files changed, 17 insertions, 8 deletions

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index df48b1c..8b1556e 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 2121fd3..3e4b572 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, 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 4bc0c49..3fd6c8d 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);