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

Christian Biesinger via gdb-patches
2020-01-14T22:13:53Z

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
>