From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104644 invoked by alias); 21 May 2019 13:08:49 -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 104623 invoked by uid 89); 21 May 2019 13:08:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=sk:cu_part, sk:find_pa, knock, sk:guess_p X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 21 May 2019 13:08:47 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 32888AEC2; Tue, 21 May 2019 13:08:45 +0000 (UTC) Subject: Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat To: Andrew Burgess Cc: Tom Tromey , gdb-patches@sourceware.org References: <20190503093124.GA27838@delia> <20190516153746.GQ2568@embecosm.com> <20190517214645.GX2568@embecosm.com> From: Tom de Vries Message-ID: <8ecb81cf-2b0e-0305-5e9a-a5313f0d24f3@suse.de> Date: Tue, 21 May 2019 13:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190517214645.GX2568@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00471.txt.bz2 On 17-05-19 23:46, Andrew Burgess wrote: > * 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. > Ah, I see what you meant now. Agreed, that's a nice cleanup. Thanks, - Tom >> >> Committed as below. > > I plan to apply the patch below to master (once its completed a test > run) unless you have any strong objections. > > 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; > >