[1/3] 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 that is used for
insertion is also used when creating the gdb_bfd_data.

gdb/ChangeLog
2020-01-14  Tom Tromey  <tromey@adacore.com>

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

Change-Id: I649ef7060651ac12188fa99d6ae1546caec93594
---
 gdb/ChangeLog |  7 +++++++
 gdb/gdb_bfd.c | 50 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 15 deletions(-)
  

On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey <tromey@adacore.com> wrote:
>
> 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 that is used for
> insertion is also used when creating the gdb_bfd_data.
>
> gdb/ChangeLog
> 2020-01-14  Tom Tromey  <tromey@adacore.com>
>
>         PR win32/25302:
>         * gdb_bfd.c (gdb_bfd_data): Add "mt" parameter.
>         (gdb_bfd_init_data): New function.
>         (gdb_bfd_open, gdb_bfd_ref): Use gdb_bfd_init_data.
>
> Change-Id: I649ef7060651ac12188fa99d6ae1546caec93594
> ---
>  gdb/ChangeLog |  7 +++++++
>  gdb/gdb_bfd.c | 50 +++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 5a6dee2d51a..a1ee7814f32 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -59,8 +59,8 @@ static htab_t all_bfds;
>
>  struct gdb_bfd_data
>  {
> -  gdb_bfd_data (bfd *abfd)
> -    : mtime (bfd_get_mtime (abfd)),
> +  gdb_bfd_data (bfd *abfd, time_t mt)
> +    : mtime (mt),
>        size (bfd_get_size (abfd)),
>        relocation_computed (0),
>        needs_relocations (0),
> @@ -376,6 +376,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, time_t mtime)
> +{
> +  struct gdb_bfd_data *gdata;
> +  void **slot;

Any reason not to move these down to where they are used?

> +  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, mtime);
> +  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
> @@ -469,7 +493,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 mtime here, rather
> +     than, say, calling gdb_bfd_ref_ptr::new_reference here.  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, search.mtime);
> +  return gdb_bfd_ref_ptr (abfd);
>  }
>
>  /* A helper function that releases any section data attached to the
> @@ -522,7 +553,6 @@ void
>  gdb_bfd_ref (struct bfd *abfd)
>  {
>    struct gdb_bfd_data *gdata;
> -  void **slot;
>
>    if (abfd == NULL)
>      return;
> @@ -541,17 +571,7 @@ 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;
> +  gdb_bfd_init_data (abfd, bfd_get_mtime (abfd));
>  }
>
>  /* See gdb_bfd.h.  */
> --
> 2.21.1
>