From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26331 invoked by alias); 14 Jan 2020 22:13:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 26323 invoked by uid 89); 14 Jan 2020 22:13:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.9 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_JMF_BL,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=HX-Received:6042 X-HELO: mail-oi1-f193.google.com Received: from mail-oi1-f193.google.com (HELO mail-oi1-f193.google.com) (209.85.167.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Jan 2020 22:13:51 +0000 Received: by mail-oi1-f193.google.com with SMTP id a67so13466742oib.6 for ; Tue, 14 Jan 2020 14:13:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=n1sH0sikwOshNE8jevgO6Xi4pzCHDObn9ZcW6t4/vUs=; b=f+Cvqy6ks0Z605I4IfkON9vxXXUHFr1d7lKKbHfMFkHMjeBHxGxbbwNCCebrmuU7mT ljgsOQDxAp0KRkQZY7QBNbWYSG1azmBBf1T70G8Z7JNLJSocCxS9iSL1or44Evgim4QJ JvVVcGCroQX9HjKD7WI3fBeIWZp4lQAZbryIkW9TUK8zsPAtGkNsOKKLUwiyC6nFLN0+ 3R2KEnWGm2c0esr2aN3fycu6bpZIVlErXTPCF8jXkqedUE4TYqVwsh+RlQldK+IEts3f fqA8P6harrLclr2dyaYG/Rgf8pXIZgQ3CgkclM0rSq9xKHOLWVpVHw47+ZxsZciZ4Gg8 8P7w== MIME-Version: 1.0 References: <20200114210956.25115-1-tromey@adacore.com> <20200114210956.25115-2-tromey@adacore.com> In-Reply-To: <20200114210956.25115-2-tromey@adacore.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Tue, 14 Jan 2020 22:26:00 -0000 Message-ID: Subject: Re: [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c To: Tom Tromey Cc: gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00391.txt.bz2 On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey 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 > > 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 >