From: Tom Tromey Date: Tue, 8 Sep 2020 16:13:51 +0000 (-0600) Subject: Avoid hash table corruption in gdb_bfd.c X-Git-Tag: gdb-10.1-release~120 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3cae444768c36314cc4acf80714461cbe0aff4e4;p=thirdparty%2Fbinutils-gdb.git 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 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 788f4e6014d..01daa99869b 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,10 @@ +2020-09-08 Tom Tromey + + 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 * infrun.c (fetch_inferior_event): Use diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 25e0178a8b8..15bf9f79c20 100644 --- 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. */