Fix reading .dwp files without .debug_tu_index

  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, 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:



Kind regards,
Alexander Shaposhnikov
  

cc: Doug Evans, Pedro Alves
Kind regards,
Alexander Shaposhnikov

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, 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;
>
>    if (dwp_file->version == 2)
>      bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,
>
>
> Kind regards,
> Alexander Shaposhnikov
>
  

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
  

>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;

i'm ok with that change, many thanks,
do i need to resend the patch ? (and yeah, i can double check / test it
when i get home)

regarding the dependence on the order of sections - (if i am not mistaken)
yeah, it's a separate issue - i can provide more details
when i get to my laptop.

Best regards,
Alexander Shaposhnikov

On Mon, Sep 25, 2017 at 8:14 PM, Doug Evans <dje@google.com> wrote:

> 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
>
  

On Mon, Sep 25, 2017 at 9:13 PM, Alexander Shaposhnikov
<alexander.v.shaposhnikov@gmail.com> wrote:
>
>  >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;
>
> i'm ok with that change, many thanks,
> do i need to resend the patch ? (and yeah, i can double check / test it when
> i get home)

No need, if you're confident all the nits are taken care of [ChangeLog
syntax and contents, patch whitespace, testing, and so on ... :-)]

> regarding the dependence on the order of sections - (if i am not mistaken)
> yeah, it's a separate issue - i can provide more details
> when i get to my laptop.

Thanks.


>
> Best regards,
> Alexander Shaposhnikov
>
> On Mon, Sep 25, 2017 at 8:14 PM, Doug Evans <dje@google.com> wrote:
>>
>> 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
>
>