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

[Alexander Shaposhnikov <alexander dot v dot shaposhnikov at gmail dot com>]

(updated patch + changelog)

changelog:

2017-09-26  Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com>
	
	* dwarf2read.c (open_and_init_dwp_file): Protect against dwp_file
	having NULL cus or tus.

patch:

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b3c5fabfc0..4e6bf27364 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -11196,7 +11196,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
@@ -11206,7 +11207,13 @@ 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;
+
+  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;

   if (dwp_file->version == 2)
     bfd_map_over_sections (dwp_file->dbfd, dwarf2_locate_v2_dwp_sections,


Kind regards,
Alexander Shaposhnikov


On Tue, Sep 26, 2017 at 10:51 AM, Doug Evans <dje@google.com> wrote:

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