From patchwork Fri May 17 21:46:45 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 32751 Received: (qmail 127821 invoked by alias); 17 May 2019 21:46:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 127813 invoked by uid 89); 17 May 2019 21:46:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 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= X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 May 2019 21:46:49 +0000 Received: by mail-wr1-f65.google.com with SMTP id f8so2150065wrt.1 for ; Fri, 17 May 2019 14:46:49 -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=Zwjp3wKvltOVUj7cBWVH5q7ghNL4sVuWFCLks05LwYs=; b=U+0klorhBPBcMfKOMUqhgmaJCssR2aEbvtpdBzMlCvtX8qWTByMuQjRkyboTp2i1ji gUe0/YNZXNTxXVax1fJpjtt9fSlD4iKN1741XAn7xpUUx2BuhbtlFNxMia3TgqSrVURB H6zcWR9f/MG5bVwKN5RdL9Wqa+lwsjyXdfhXXDN4JMw2NauxJua7A4nUSn0u65X6l08v pMOS8Anzj0R1bWwPqI/xKVbrZdRPiG4geA6WgbEfapcRrIIOiip8qTiKRIGoKxmnjq+s 7QfOxtjno7UpEIAHNHWAIcBE8VZwO420kwqgaNoibNRPi80vsxlq70mlmrV0xVq9Pt2j Xlhw== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id f2sm10706362wme.12.2019.05.17.14.46.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 17 May 2019 14:46:46 -0700 (PDT) Date: Fri, 17 May 2019 22:46:45 +0100 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: <20190517214645.GX2568@embecosm.com> References: <20190503093124.GA27838@delia> <20190516153746.GQ2568@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: We are using Linux daily to UP our productivity -- so UP yours, Microsoft! X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * 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. 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;