From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98423 invoked by alias); 24 Feb 2019 04:01: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 98179 invoked by uid 89); 24 Feb 2019 04:01:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=highest, increment, H*c:US-ASCII, explanation X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 24 Feb 2019 04:01:15 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id x1O416ic030379 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sat, 23 Feb 2019 23:01:11 -0500 Received: by simark.ca (Postfix, from userid 112) id 209941E658; Sat, 23 Feb 2019 23:01:06 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 93C2E1E477; Sat, 23 Feb 2019 23:01:04 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sun, 24 Feb 2019 04:01:00 -0000 From: Simon Marchi To: Jordan Rupprecht Cc: gdb-patches@sourceware.org, echristo@gmail.com, dblaikie@gmail.com, dje@google.com Subject: Re: [PATCH] [dwarf2read] Fix crash when loading dwp files: calculate num_sections based on actual section indices, not just the number of sections. In-Reply-To: <20190223005323.188851-1-rupprecht@google.com> References: <20190223005323.188851-1-rupprecht@google.com> Message-ID: <439bf4f6859a539472a2e51a028b9503@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00389.txt.bz2 On 2019-02-22 19:53, Jordan Rupprecht via gdb-patches wrote: > When loading dwp files, we create an array of elf sections indexed by > the section index in the dwp file. The size of this array is > calculated by section_count + 1 (the +1 handling the null section). > However, when loading the bfd file, strtab/symtab sections are not > added to the list, nor do they increment section_count, so > section_count is actually smaller than the number of sections. Just wondering, is this the expected behavior of BFD, to not make the strtab section count as a section (as far as bfd_count_sections is concerned)? If so, why? Otherwise can we just elf_numsections instead of bfd_count_sections? Since we index the array by ELF section index, using the number of ELF sections seems appropriate, it should always match. We wouldn't need the +1 then. > This happens to work when using GNU dwp, which lays out .debug section > first, with sections like .shstrtab coming at the end. Other tools, > like llvm-dwp, put .strtab first, and gdb crashes when loading those > dwp files. > > For instance, with the current state of gdb, loading a file like this: > $ readelf -SW > [ 0] > [ 1] .debug_foo PROGBITS ... > [ 2] .strtab STRTAB ... > > ... results in section_count = 2 (.debug is the only thing placed into > bfd->sections, so section_count + 1 == 2), and sectp->this_idx = 1 > when mapping over .debug_foo in dwarf2_locate_common_dwp_sections, > which passes the assertion that 1 < 2. > > However, using a dwp file produced by llvm-dwp: > $ readelf -SW > [ 0] > [ 1] .strtab STRTAB ... > [ 2] .debug_foo PROGBITS ... > ... results in section_count = 2 (.debug is the only thing placed into > bfd->sections, so section_count + 1 == 2), and sectp->this_idx = 2 > when mapping over .debug_foo in dwarf2_locate_common_dwp_sections, > which fails the assertion that 2 < 2. > > This patch changes the calculation of section_count to look at the > actual highest this_idx value we see instead of assuming that all > strtab/symtab sections come at the end. Thanks for the detailed explanation. Here are just some formatting comments. > --- > gdb/ChangeLog | 6 ++++++ > gdb/dwarf2read.c | 17 +++++++++++++++-- > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 6a551dfa64..c789b34890 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,9 @@ > +2019-02-22 Jordan Rupprecht > + > + * dwarf2read.c (get_largest_section_index): new function Capital letter and period at the end ("New function."). > + (open_and_init_dwp_file): Call get_largest_section_index to > + calculate dwp_file->num_sections. > + > 2019-02-22 Simon Marchi > > * MAINTAINERS: Update my email address. > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 98f46e0416..07a9d4ea5d 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -13182,6 +13182,18 @@ open_dwp_file (struct dwarf2_per_objfile > *dwarf2_per_objfile, > return NULL; > } > > +/* Return the largest section index */ Period at the end, followed by two spaces: /* Return the largest section index. */ > +static int > +get_largest_section_index(bfd *bfd) Space before the parentheses: get_largest_section_index (bfd *bfd) > +{ > + int max_sec_idx = 0; > + asection *sectp; You can declare sectp in the for loop header. > + for (sectp = bfd->sections; sectp != NULL; sectp = sectp->next) > + max_sec_idx = std::max (max_sec_idx, elf_section_data > (sectp)->this_idx); > + > + return max_sec_idx; > +} > + > /* Initialize the use of the DWP file for the current objfile. > By convention the name of the DWP file is ${objfile}.dwp. > The result is NULL if it can't be found. */ > @@ -13230,8 +13242,9 @@ open_and_init_dwp_file (struct > dwarf2_per_objfile *dwarf2_per_objfile) > std::unique_ptr dwp_file > (new struct dwp_file (name, std::move (dbfd))); > > - /* +1: section 0 is unused */ > - dwp_file->num_sections = bfd_count_sections (dwp_file->dbfd) + 1; > + /* +1: so that dwp_file->elf_sections[max_idx] is valid */ Period and two spaces at the end. > + dwp_file->num_sections = > + get_largest_section_index (dwp_file->dbfd.get()) + 1; Normally, the = should be on the second line when breaking assignments (despite the counter example just below). But in this case, it can fit on a single line I believe. > dwp_file->elf_sections = > OBSTACK_CALLOC (&objfile->objfile_obstack, > dwp_file->num_sections, asection *); Thanks, Simon