Re: [PATCH] Fix reading .dwp files without .debug_tu_index

["Doug Evans via gdb-patches" <gdb-patches@sourceware.org>]

On Tue, Sep 19, 2017 at 12:58 PM, Alexander Shaposhnikov
<alexander.v.shaposhnikov@gmail.com> wrote:
> This patch fixes segmentation fault (due to dereferencing of a null pointer)
> in dwarf2read.c in the code dwp_file->cus->version != dwp_file->tus->version
> by adding defensive checks similarly to how it's already done
> at the lines 11208 - 11215 (in the same file dwarf2read.c).
> The issue can be reproduced with dwp packages built by llvm-dwp utility
> (from the current upstream) since at the moment llvm-dwp doesn't create
> .debug_tu_index section, thus dwp_file->tus will be null.
>
> Test plan:
>
> BUILD:
> main.cpp:
> int f() {
>   int x = 0;
>   int y = 1;
>   return x + y;
> }
> g++ -fPIC -gsplit-dwarf -g -O0 main.cpp -o main.exe
> llvm-dwp main.dwo -o main.exe.dwp
> # This step is a workaround to a separate issue (unrelated to this patch).
> # At the moment llvm tools & clang put .strtab section first (its index is
> 1),
> # while gcc/gold/binutils put it at the end.
> # Unfortunately gdb (in the code reading dwps) appears to depend on the
> order
> # of sections,

That's odd. Depends on the order of sections how?

> to workaround this (to reproduce the issue which this patch
> # aims to address) we use objcopy to do the following trick:
> # if one asks to remove .strtab objcopy will do that but at the same time
> # it will create a new .shstrtab section at the end.
> objcopy --remove-section .strtab main.exe.dwp
> RUN:
> gdb main.exe
> One can observe that now there is no crash and debugging functionality
> works as expected (setting breakpoints, printing local variable, exploring
> the stack).
>
> gdb/ChangeLog:
> yyyy-mm-dd  Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com>
>
> * dwarf2read.c: Fix segmentation fault on reading
>
>   .dwp files without .debug_tu_index section.
>
> Patch:
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>
> index b1914cf876..547e3f034e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -11185,7 +11185,8 @@ open_and_init_dwp_file (void)
>    dwp_file->tus = create_dwp_hash_table (dwp_file, 1);
>
>    /* The DWP file version is stored in the hash table.  Oh well.  */
> -  if (dwp_file->cus->version != dwp_file->tus->version)
> +  if (dwp_file->cus && dwp_file->tus
> +      && dwp_file->cus->version != dwp_file->tus->version)
>      {
>        /* Technically speaking, we should try to limp along, but this is
>          pretty bizarre.  We use pulongest here because that's the established
> @@ -11195,7 +11196,7 @@ open_and_init_dwp_file (void)
>              pulongest (dwp_file->cus->version),
>              pulongest (dwp_file->tus->version), dwp_name.c_str ());
>      }
> -  dwp_file->version = dwp_file->cus->version;
> +  dwp_file->version = dwp_file->cus ? dwp_file->cus->version : 0;

A version of 0 doesn't seem right.
I'd go with something like (untested and likely improperly formatted):

  if (dwp_file->cus)
    dwp_file->version = dwp_file-cus->version;
  else if (dwp_file->tus)
    dwp_file->version = dwp_file->tus->version
  else
    dwp_file->version = 2;

ok with that change.

>
>    if (dwp_file->version == 2)
>      bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,
>
>
> Kind regards,
> Alexander Shaposhnikov