From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101177 invoked by alias); 18 May 2019 08:51:30 -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 101169 invoked by uid 89); 18 May 2019 08:51:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Received:Sat X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 18 May 2019 08:51:28 +0000 Received: by mail-wr1-f66.google.com with SMTP id b18so9350593wrq.12 for ; Sat, 18 May 2019 01:51:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=57b+2rK/PH6m8k+/7jsw3/b2qjGhFWMteJDp07/iVJE=; b=U2N8BO6OpBhQqMUW60ad1iR8kVlG7CxtJo2VjZIgZ8BJ/oaNqiiwqMtFlMo7Ghlx94 1tvlA9PWh56TvGywTXBBQgrO2eangYZCK2sC/TWoBVd3IxEvE5JG7Mzpjhl0XB0aVlQV K/mHLObZWkgMS1/lFrUPrUMFI0nRZpwEKCwT/28mcsPtUmINkM/0u2Ypt+L+x1uK6smB MbPsLgkjui3y2rDX7In4cSJl48b521qnb7GfczsjaEYpFbYNlLq0H6b+CbOs/2bh0QH/ IyK+T5qPbNsZA3nejdbVBWYzyhkNk0N6RePiNIz4mLgeWt9NiY+Ay7SDzDBmxDIIX3oi 6dVQ== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id q24sm10588191wmc.18.2019.05.18.01.51.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 18 May 2019 01:51:25 -0700 (PDT) Date: Sat, 18 May 2019 08:51:00 -0000 From: Andrew Burgess To: Tom de Vries Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat Message-ID: <20190518085124.GY2568@embecosm.com> References: <20190503093124.GA27838@delia> <20190516153746.GQ2568@embecosm.com> <20190517214645.GX2568@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190517214645.GX2568@embecosm.com> X-Fortune: War is an equal opportunity destroyer. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00438.txt.bz2 * Andrew Burgess [2019-05-17 22:46:45 +0100]: > * Tom de Vries [2019-05-17 09:40:57 +0200]: > > > On 16-05-19 17:37, Andrew Burgess wrote: > > > * Tom de Vries [2019-05-03 11:31:26 +0200]: > > > > > This all sounds good. I have a couple of small suggestions inline > > > below... > > > > > >> > > >> --- > > >> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++---------- > > >> 1 file changed, 39 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > > >> index b0bdecf96f..442b618f6e 100644 > > >> --- a/gdb/dwarf2read.c > > >> +++ b/gdb/dwarf2read.c > > >> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *); > > >> static struct partial_die_info *load_partial_dies > > >> (const struct die_reader_specs *, const gdb_byte *, int); > > >> > > >> -static struct partial_die_info *find_partial_die (sect_offset, int, > > >> - struct dwarf2_cu *); > > >> +struct cu_partial_die_info > > >> +{ > > >> + struct dwarf2_cu *cu; > > >> + struct partial_die_info *pdi; > > >> +}; > > > > > > This needs at least a header comment describing its use, and ideally > > > each field documented too. > > > > > > > Done. > > > > > I wonder though if you should also provide this with a 2 argument > > > constructor, and delete the default constructor, like: > > > > > > /* blah blah blah... */ > > > > > > struct cu_partial_die_info > > > { > > > /* mumble.. */ > > > struct dwarf2_cu *cu; > > > > > > /* mutter... */ > > > struct partial_die_info *pdi; > > > > > > cu_partial_die_info (struct dwarf2_cu *cu, > > > struct partial_die_info *pdi) > > > : cu (cu), > > > pdi (pdi) > > > { /* Nothing. */ } > > > > > > private: > > > cu_partial_die_info () = delete; > > > }; > > > > > > This will lead to some obvious knock on changes in the rest of the > > > code, which I think are probably improvements. > > > > > > > I've tried this out, and the only effect was this type of changes: > > ... > > - struct cu_partial_die_info res; > > + struct cu_partial_die_info res (NULL, NULL); > > ... > > So, I've left this out for now. > > Sorry, I didn't explain myself very well. > > > > > Committed as below. > > I plan to apply the patch below to master (once its completed a test > run) unless you have any strong objections. Passed testing, so pushed. Thanks, Andrew > > Thanks, > Andrew > > --- > > [PATCH] gdb: Add constructor to struct cu_partial_die_info > > Adds a constructor to 'struct cu_partial_die_info' and disables the > default constructor, preventing partially initialised instances from > being created. > > Update 'find_partial_die' to return a const struct. > > Users of 'find_partial_die' are updated to take account of the above > two changes. > > There should be no user visible changes after this commit. > > gdb/ChangeLog: > > * dwarf2read.c (struct cu_partial_die_info): Add constructor, > delete default constructor. > (find_partial_die): Update to return const struct. > (partial_die_parent_scope): Move variable declaration into scope > of its use and change its type to auto. > (guess_partial_die_structure_name): Likewise. > (partial_die_info::fixup): Likewise. > --- > gdb/ChangeLog | 10 ++++++++++ > gdb/dwarf2read.c | 27 ++++++++++++++++----------- > 2 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 004238a6775..f48b931a3f3 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -1514,10 +1514,18 @@ struct cu_partial_die_info > struct dwarf2_cu *cu; > /* A partial_die_info. */ > struct partial_die_info *pdi; > + > + cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi) > + : cu (cu), > + pdi (pdi) > + { /* Nothhing. */ } > + > +private: > + cu_partial_die_info () = delete; > }; > > -static struct cu_partial_die_info find_partial_die (sect_offset, int, > - struct dwarf2_cu *); > +static const struct cu_partial_die_info find_partial_die (sect_offset, int, > + struct dwarf2_cu *); > > static const gdb_byte *read_attribute (const struct die_reader_specs *, > struct attribute *, struct attr_abbrev *, > @@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi, > { > const char *grandparent_scope; > struct partial_die_info *parent, *real_pdi; > - struct cu_partial_die_info res; > > /* We need to look at our parent DIE; if we have a DW_AT_specification, > then this means the parent of the specification DIE. */ > @@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi, > real_pdi = pdi; > while (real_pdi->has_specification) > { > - res = find_partial_die (real_pdi->spec_offset, > - real_pdi->spec_is_dwz, cu); > + auto res = find_partial_die (real_pdi->spec_offset, > + real_pdi->spec_is_dwz, cu); > real_pdi = res.pdi; > cu = res.cu; > } > @@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off) > outside their CU (they do however referencing other types via > DW_FORM_ref_sig8). */ > > -static struct cu_partial_die_info > +static const struct cu_partial_die_info > find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu) > { > struct dwarf2_per_objfile *dwarf2_per_objfile > @@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi, > > struct partial_die_info *real_pdi; > struct partial_die_info *child_pdi; > - struct cu_partial_die_info res; > > /* If this DIE (this DIE's specification, if any) has a parent, then > we should not do this. We'll prepend the parent's fully qualified > @@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi, > real_pdi = struct_pdi; > while (real_pdi->has_specification) > { > - res = find_partial_die (real_pdi->spec_offset, > - real_pdi->spec_is_dwz, cu); > + auto res = find_partial_die (real_pdi->spec_offset, > + real_pdi->spec_is_dwz, cu); > real_pdi = res.pdi; > cu = res.cu; > } > @@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu) > if (name == NULL && has_specification) > { > struct partial_die_info *spec_die; > - struct cu_partial_die_info res; > > - res = find_partial_die (spec_offset, spec_is_dwz, cu); > + auto res = find_partial_die (spec_offset, spec_is_dwz, cu); > spec_die = res.pdi; > cu = res.cu; > > -- > 2.14.5 >