Re: [PATCH] [dwarf2read] Fix crash when loading dwp files: calculate num_sections based on actual section indices, not just the number of sections.

[Simon Marchi <simon.marchi@polymtl.ca>]

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