This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] [dwarf2read] Fix crash when loading dwp files: calculate num_sections based on actual section indices, not just the number of sections.
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Jordan Rupprecht <rupprecht at google dot com>
- Cc: gdb-patches at sourceware dot org, echristo at gmail dot com, dblaikie at gmail dot com, dje at google dot com
- Date: Sat, 23 Feb 2019 23:01:04 -0500
- Subject: Re: [PATCH] [dwarf2read] Fix crash when loading dwp files: calculate num_sections based on actual section indices, not just the number of sections.
- References: <20190223005323.188851-1-rupprecht@google.com>
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 <file.dwp>
[ 0] <empty>
[ 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 <file.dwp>
[ 0] <empty>
[ 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 <rupprecht@google.com>
+
+ * 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 <simon.marchi@polymtl.ca>
* 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<struct dwp_file> 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