Re: [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

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
>