Avoid hash table corruption in gdb_bfd.c

gdb caches BFDs that come from ordinary files.  This code turns out to
have a bug where the hash table can become corrupted, causing gdb to
crash.

When gdb_bfd_open opens the BFD, it uses fstat to get the BFD's mtime.
This is used when inserting the entry into gdb_bfd_cache.  Then, the
function creates the gdb_bfd_data object as a side effect of calling
new_reference.  This object is used when finding objects in the hash
table, and its constructor uses bfd_get_mtime.  So, if the file
changes between the time the BFD is put into the cache and the time
that this object is created, the hash table will be incorrect.  When
the BFD is later deleted, its entry in the hash table will not be
found, and at this point the hash table will point to invalid memory.

This patch fixes the bug by ensuring that the mtime, and other
relevant attributes comgin from stat, that are used for insertion are
also used when creating the gdb_bfd_data.

This obsoletes an earlier patch that had split this into two parts
(surrounding a patch to use bfd_stat more consistently).  This version
merges the two patches, in the interest of correctness.

gdb/ChangeLog
2020-09-08  Tom Tromey  <tromey@adacore.com>

	PR win32/25302:
	* gdb_bfd.c (gdb_bfd_data): Add "st" parameter.
	(gdb_bfd_init_data): New function.
	(gdb_bfd_open, gdb_bfd_ref): Use gdb_bfd_init_data.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 788f4e6014d6ef36a9ebd9097b7af34f16316d01..01daa99869ba26cfb2a9d24e62d6739f03f703b4 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2020-09-08  Tom Tromey  <tromey@adacore.com>
+
+       PR win32/25302:
+       * gdb_bfd.c (gdb_bfd_data): Add "st" parameter.
+       (gdb_bfd_init_data): New function.
+       (gdb_bfd_open, gdb_bfd_ref): Use gdb_bfd_init_data.
+
 2020-09-07  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
 
        * infrun.c (fetch_inferior_event): Use

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 25e0178a8b8beb34e71c3d667db199227e42b569..15bf9f79c2038933b84175e1ead398f178687779 100644 (file)
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -59,26 +59,16 @@ static htab_t all_bfds;
 
 struct gdb_bfd_data
 {
-  gdb_bfd_data (bfd *abfd)
-    : mtime (bfd_get_mtime (abfd)),
-      size (bfd_get_size (abfd)),
+  /* Note that if ST is nullptr, then we simply fill in zeroes.  */
+  gdb_bfd_data (bfd *abfd, struct stat *st)
+    : mtime (st == nullptr ? 0 : st->st_mtime),
+      size (st == nullptr ? 0 : st->st_size),
+      inode (st == nullptr ? 0 : st->st_ino),
+      device_id (st == nullptr ? 0 : st->st_dev),
       relocation_computed (0),
       needs_relocations (0),
       crc_computed (0)
   {
-    struct stat buf;
-
-    if (bfd_stat (abfd, &buf) == 0)
-      {
-       inode = buf.st_ino;
-       device_id = buf.st_dev;
-      }
-    else
-      {
-       /* The stat failed.  */
-       inode = 0;
-       device_id = 0;
-      }
   }
 
   ~gdb_bfd_data ()
@@ -382,6 +372,30 @@ gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream,
   return result;
 }
 
+/* A helper function to initialize the data that gdb attaches to each
+   BFD.  */
+
+static void
+gdb_bfd_init_data (struct bfd *abfd, struct stat *st)
+{
+  struct gdb_bfd_data *gdata;
+  void **slot;
+
+  gdb_assert (bfd_usrdata (abfd) == nullptr);
+
+  /* Ask BFD to decompress sections in bfd_get_full_section_contents.  */
+  abfd->flags |= BFD_DECOMPRESS;
+
+  gdata = new gdb_bfd_data (abfd, st);
+  bfd_set_usrdata (abfd, gdata);
+  bfd_alloc_data (abfd);
+
+  /* This is the first we've seen it, so add it to the hash table.  */
+  slot = htab_find_slot (all_bfds, abfd, INSERT);
+  gdb_assert (slot && !*slot);
+  *slot = abfd;
+}
+
 /* See gdb_bfd.h.  */
 
 gdb_bfd_ref_ptr
@@ -426,23 +440,23 @@ gdb_bfd_open (const char *name, const char *target, int fd,
        }
     }
 
-  search.filename = name;
   if (fstat (fd, &st) < 0)
     {
-      /* Weird situation here.  */
-      search.mtime = 0;
-      search.size = 0;
-      search.inode = 0;
-      search.device_id = 0;
-    }
-  else
-    {
-      search.mtime = st.st_mtime;
-      search.size = st.st_size;
-      search.inode = st.st_ino;
-      search.device_id = st.st_dev;
+      /* Weird situation here -- don't cache if we can't stat.  */
+      if (debug_bfd_cache)
+       fprintf_unfiltered (gdb_stdlog,
+                           "Could not stat bfd %s for %s - not caching\n",
+                           host_address_to_string (abfd),
+                           bfd_get_filename (abfd));
+      return gdb_bfd_ref_ptr::new_reference (abfd);
     }
 
+  search.filename = name;
+  search.mtime = st.st_mtime;
+  search.size = st.st_size;
+  search.inode = st.st_ino;
+  search.device_id = st.st_dev;
+
   /* Note that this must compute the same result as hash_bfd.  */
   hash = htab_hash_string (name);
   /* Note that we cannot use htab_find_slot_with_hash here, because
@@ -477,7 +491,14 @@ gdb_bfd_open (const char *name, const char *target, int fd,
       *slot = abfd;
     }
 
-  return gdb_bfd_ref_ptr::new_reference (abfd);
+  /* It's important to pass the already-computed stat info here,
+     rather than, say, calling gdb_bfd_ref_ptr::new_reference.  BFD by
+     default will "stat" the file each time bfd_get_mtime is called --
+     and since we already entered it into the hash table using this
+     mtime, if the file changed at the wrong moment, the race would
+     lead to a hash table corruption.  */
+  gdb_bfd_init_data (abfd, &st);
+  return gdb_bfd_ref_ptr (abfd);
 }
 
 /* A helper function that releases any section data attached to the
@@ -530,7 +551,6 @@ void
 gdb_bfd_ref (struct bfd *abfd)
 {
   struct gdb_bfd_data *gdata;
-  void **slot;
 
   if (abfd == NULL)
     return;
@@ -549,17 +569,9 @@ gdb_bfd_ref (struct bfd *abfd)
       return;
     }
 
-  /* Ask BFD to decompress sections in bfd_get_full_section_contents.  */
-  abfd->flags |= BFD_DECOMPRESS;
-
-  gdata = new gdb_bfd_data (abfd);
-  bfd_set_usrdata (abfd, gdata);
-  bfd_alloc_data (abfd);
-
-  /* This is the first we've seen it, so add it to the hash table.  */
-  slot = htab_find_slot (all_bfds, abfd, INSERT);
-  gdb_assert (slot && !*slot);
-  *slot = abfd;
+  /* Caching only happens via gdb_bfd_open, so passing nullptr here is
+     fine.  */
+  gdb_bfd_init_data (abfd, nullptr);
 }
 
 /* See gdb_bfd.h.  */