MIPS patch for crash during elf load

MIPS patch for crash during elf load

[Lightning]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

GDB has a bug with MIPS binaries where loading ones with a large number of
global objects in the debug sections results in using uninitialized memory
resulting in a crash. This was seen during a debug build of the following
project: https://github.com/pmret/papermario

I downloaded GDB 11.1 and compiled it with the following options:
./configure --target=mips-linux-gnu --program-prefix=mips-linux-gnu-

My local gcc compiler is gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

I traced the issue down to the initialization of the fdt_to_pst buffer. It
is initialized in gdb/mdebugread.c on line 2369 via a gdb::def_vector
however the internal values are never cleared.  On line 2403 a struct
variable in this buffer is incremented for the number of globals. With
enough globals and sections this can result in having invalid data in the
global field causing a crash. In the above case it was stale pointer data.
The fix is the attached patch, a simple memset after the buffer is
initialized.

Jewell

[-- Attachment #2: gdb-11.1.mips.patch --]
[-- Type: application/octet-stream, Size: 486 bytes --]

--- gdb-11.1.old/gdb/mdebugread.c	2021-07-03 10:41:09.000000000 -0700
+++ gdb-11.1/gdb/mdebugread.c	2021-11-19 20:57:23.662003100 -0800
@@ -2367,6 +2367,7 @@
      to FDR -1.  We`ll go along with that.  */
   gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
   fdr_to_pst = fdr_to_pst_holder.data ();
+  memset(fdr_to_pst, 0, sizeof(struct pst_map) * (hdr->ifdMax+1));
   fdr_to_pst++;
   {
     legacy_psymtab *new_pst = new_psymtab ("", partial_symtabs, objfile);

Re: MIPS patch for crash during elf load

[Lightning]

Text version of the patch being it was scrubbed on the mailing list, I
didn't expect the attachment to show up as non-text.

--- gdb-11.1.old/gdb/mdebugread.c       2021-07-03 10:41:09.000000000 -0700
+++ gdb-11.1/gdb/mdebugread.c   2021-11-19 20:57:23.662003100 -0800
@@ -2367,6 +2367,7 @@
      to FDR -1.  We`ll go along with that.  */
   gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
   fdr_to_pst = fdr_to_pst_holder.data ();
+       memset(fdr_to_pst, 0, sizeof(struct pst_map) * (hdr->ifdMax+1));
   fdr_to_pst++;
   {
     legacy_psymtab *new_pst = new_psymtab ("", partial_symtabs, objfile);

On Mon, Nov 22, 2021 at 8:50 PM Lightning <lightningth@gmail.com> wrote:

> GDB has a bug with MIPS binaries where loading ones with a large number of
> global objects in the debug sections results in using uninitialized memory
> resulting in a crash. This was seen during a debug build of the following
> project: https://github.com/pmret/papermario
>
> I downloaded GDB 11.1 and compiled it with the following options:
> ./configure --target=mips-linux-gnu --program-prefix=mips-linux-gnu-
>
> My local gcc compiler is gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
>
> I traced the issue down to the initialization of the fdt_to_pst buffer. It
> is initialized in gdb/mdebugread.c on line 2369 via a gdb::def_vector
> however the internal values are never cleared.  On line 2403 a struct
> variable in this buffer is incremented for the number of globals. With
> enough globals and sections this can result in having invalid data in the
> global field causing a crash. In the above case it was stale pointer data.
> The fix is the attached patch, a simple memset after the buffer is
> initialized.
>
> Jewell
>

Re: MIPS patch for crash during elf load

[Andrew Burgess]

* Lightning via Gdb-patches <gdb-patches@sourceware.org> [2021-11-22 20:50:03 -0800]:

> GDB has a bug with MIPS binaries where loading ones with a large number of
> global objects in the debug sections results in using uninitialized memory
> resulting in a crash. This was seen during a debug build of the following
> project: https://github.com/pmret/papermario
> 
> I downloaded GDB 11.1 and compiled it with the following options:
> ./configure --target=mips-linux-gnu --program-prefix=mips-linux-gnu-
> 
> My local gcc compiler is gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> 
> I traced the issue down to the initialization of the fdt_to_pst buffer. It
> is initialized in gdb/mdebugread.c on line 2369 via a gdb::def_vector
> however the internal values are never cleared.  On line 2403 a struct
> variable in this buffer is incremented for the number of globals. With
> enough globals and sections this can result in having invalid data in the
> global field causing a crash. In the above case it was stale pointer data.
> The fix is the attached patch, a simple memset after the buffer is
> initialized.

Thanks for looking into this issue.

I didn't really understand the back half of your problem description
here.

I understand "On line 2403 a struct variable in this buffer is
incremented for the number of globals." and this looks bad, but then
you say, "With enough globals and sections this can result in having
invalid data in the global field causing a crash."

I don't understand what the "enough globals" bit is about?

As the gdb::def_vector doesn't initialize the backing memory, surely
we can just say something like: On line 2403 a struct variable in this
buffer is incremented for the number of globals, and as the memory was
not zero initialized the result of the increment is undefined.  This
can lead to undefined behaviour later on, which can result in a crash?

Or have I missed some subtle detail of this issue?

Also your subject line talks about mips and elf, but doesn't mention
the critical component here, ECOFF debug information, maybe: "fix crash
when reading ECOFF debug information" would be more on topic?

On the fix itself, gdb::def_vector is a variation on std::vector
specifically designed NOT to initialize the backing memory.  Given we
are no immediately initializing the memory, I wonder if switching to
std::vector would be better?

I brought all this together in the patch below.  As I don't have any
way to get this, I wonder if you'd be willing to give this patch a try
please,

Thanks,
Andrew

----

commit 7c3dae84e2051aff97dd1da1115c476a51072808
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Nov 22 20:52:15 2021 -0800

    gdb: fix crash when reading ECOFF debug information
    
    In commit:
    
      commit 633cf2548bcd3dafe297e21a1dd3574240280d48
      Date:   Wed May 9 15:42:28 2018 -0600
    
          Remove cleanups from mdebugread.c
    
    the following change was made in the function parse_partial_symbols in
    mdebugread.c:
    
      -  fdr_to_pst = XCNEWVEC (struct pst_map, hdr->ifdMax + 1);
      -  old_chain = make_cleanup (xfree, fdr_to_pst);
      +  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
      +  fdr_to_pst = fdr_to_pst_holder.data ();
    
    The problem with this change is that XCNEWVEC calls xcalloc, which in
    turn calls calloc, and calloc zero initializes the allocated memory.
    In contrast, the new line gdb::def_vector<struct pst_map> specifically
    does not initialize the underlying memory.
    
    This is a problem because, later on in this same function, we
    increment the n_globals field within 'struct pst_map' objects stored
    in the vector.  The incrementing is now being done from an
    uninitialized starting point.
    
    In this commit we switch from using gdb::def_vector to using
    std::vector, this alone should be enough to ensure that the fields are
    initialized to zero.
    
    However, for extra clarity, I have also added initial values in the
    'struct pst_map' to make it crystal clear how the struct will start
    up.
    
    This issue was reported on the mailing list here:
    
      https://sourceware.org/pipermail/gdb-patches/2021-November/183693.html
    
    Co-Authored-By: Lightning <lightningth@gmail.com>

diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 0faff5f43c8..9204d3debe6 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -386,9 +386,9 @@ mdebug_build_psymtabs (minimal_symbol_reader &reader,
 
 struct pst_map
 {
-  legacy_psymtab *pst;	/* the psymtab proper */
-  long n_globals;		/* exported globals (external symbols) */
-  long globals_offset;		/* cumulative */
+  legacy_psymtab *pst = nullptr;  /* the psymtab proper */
+  long n_globals = 0;		  /* exported globals (external symbols) */
+  long globals_offset = 0;	  /* cumulative */
 };
 
 
@@ -2365,7 +2365,7 @@ parse_partial_symbols (minimal_symbol_reader &reader,
   /* Allocate the map FDR -> PST.
      Minor hack: -O3 images might claim some global data belongs
      to FDR -1.  We`ll go along with that.  */
-  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
+  std::vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
   fdr_to_pst = fdr_to_pst_holder.data ();
   fdr_to_pst++;
   {

Re: MIPS patch for crash during elf load

[Lightning]

 You didn't miss anything subtle and I did fail to point out it is part of
the ECOFF parsing. I wasn't 100% sure how everything was tied together and
if swapping to the std::vector was acceptable so I tried to be clear enough
about how the issue pops up and figured if there was a better patch it
would be pointed out. Interestingly I don't see the issue show up on
smaller binaries, just on larger ones where more memory has been reused
hence my extra statement as I was trying to avoid the "I can't replicate"
situation.

I can confirm the patch you provided works just as well. Now I'm off to
figure out why I'm getting abbrev 111 CU errors.

Jewell

On Tue, Nov 23, 2021 at 7:13 AM Andrew Burgess <aburgess@redhat.com> wrote:

> * Lightning via Gdb-patches <gdb-patches@sourceware.org> [2021-11-22
> 20:50:03 -0800]:
>
> > GDB has a bug with MIPS binaries where loading ones with a large number
> of
> > global objects in the debug sections results in using uninitialized
> memory
> > resulting in a crash. This was seen during a debug build of the following
> > project: https://github.com/pmret/papermario
> >
> > I downloaded GDB 11.1 and compiled it with the following options:
> > ./configure --target=mips-linux-gnu --program-prefix=mips-linux-gnu-
> >
> > My local gcc compiler is gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> >
> > I traced the issue down to the initialization of the fdt_to_pst buffer.
> It
> > is initialized in gdb/mdebugread.c on line 2369 via a gdb::def_vector
> > however the internal values are never cleared.  On line 2403 a struct
> > variable in this buffer is incremented for the number of globals. With
> > enough globals and sections this can result in having invalid data in the
> > global field causing a crash. In the above case it was stale pointer
> data.
> > The fix is the attached patch, a simple memset after the buffer is
> > initialized.
>
> Thanks for looking into this issue.
>
> I didn't really understand the back half of your problem description
> here.
>
> I understand "On line 2403 a struct variable in this buffer is
> incremented for the number of globals." and this looks bad, but then
> you say, "With enough globals and sections this can result in having
> invalid data in the global field causing a crash."
>
> I don't understand what the "enough globals" bit is about?
>
> As the gdb::def_vector doesn't initialize the backing memory, surely
> we can just say something like: On line 2403 a struct variable in this
> buffer is incremented for the number of globals, and as the memory was
> not zero initialized the result of the increment is undefined.  This
> can lead to undefined behaviour later on, which can result in a crash?
>
> Or have I missed some subtle detail of this issue?
>
> Also your subject line talks about mips and elf, but doesn't mention
> the critical component here, ECOFF debug information, maybe: "fix crash
> when reading ECOFF debug information" would be more on topic?
>
> On the fix itself, gdb::def_vector is a variation on std::vector
> specifically designed NOT to initialize the backing memory.  Given we
> are no immediately initializing the memory, I wonder if switching to
> std::vector would be better?
>
> I brought all this together in the patch below.  As I don't have any
> way to get this, I wonder if you'd be willing to give this patch a try
> please,
>
> Thanks,
> Andrew
>
> ----
>
> commit 7c3dae84e2051aff97dd1da1115c476a51072808
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Nov 22 20:52:15 2021 -0800
>
>     gdb: fix crash when reading ECOFF debug information
>
>     In commit:
>
>       commit 633cf2548bcd3dafe297e21a1dd3574240280d48
>       Date:   Wed May 9 15:42:28 2018 -0600
>
>           Remove cleanups from mdebugread.c
>
>     the following change was made in the function parse_partial_symbols in
>     mdebugread.c:
>
>       -  fdr_to_pst = XCNEWVEC (struct pst_map, hdr->ifdMax + 1);
>       -  old_chain = make_cleanup (xfree, fdr_to_pst);
>       +  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax +
> 1);
>       +  fdr_to_pst = fdr_to_pst_holder.data ();
>
>     The problem with this change is that XCNEWVEC calls xcalloc, which in
>     turn calls calloc, and calloc zero initializes the allocated memory.
>     In contrast, the new line gdb::def_vector<struct pst_map> specifically
>     does not initialize the underlying memory.
>
>     This is a problem because, later on in this same function, we
>     increment the n_globals field within 'struct pst_map' objects stored
>     in the vector.  The incrementing is now being done from an
>     uninitialized starting point.
>
>     In this commit we switch from using gdb::def_vector to using
>     std::vector, this alone should be enough to ensure that the fields are
>     initialized to zero.
>
>     However, for extra clarity, I have also added initial values in the
>     'struct pst_map' to make it crystal clear how the struct will start
>     up.
>
>     This issue was reported on the mailing list here:
>
>
> https://sourceware.org/pipermail/gdb-patches/2021-November/183693.html
>
>     Co-Authored-By: Lightning <lightningth@gmail.com>
>
> diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
> index 0faff5f43c8..9204d3debe6 100644
> --- a/gdb/mdebugread.c
> +++ b/gdb/mdebugread.c
> @@ -386,9 +386,9 @@ mdebug_build_psymtabs (minimal_symbol_reader &reader,
>
>  struct pst_map
>  {
> -  legacy_psymtab *pst; /* the psymtab proper */
> -  long n_globals;              /* exported globals (external symbols) */
> -  long globals_offset;         /* cumulative */
> +  legacy_psymtab *pst = nullptr;  /* the psymtab proper */
> +  long n_globals = 0;            /* exported globals (external symbols) */
> +  long globals_offset = 0;       /* cumulative */
>  };
>
>
> @@ -2365,7 +2365,7 @@ parse_partial_symbols (minimal_symbol_reader &reader,
>    /* Allocate the map FDR -> PST.
>       Minor hack: -O3 images might claim some global data belongs
>       to FDR -1.  We`ll go along with that.  */
> -  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
> +  std::vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
>    fdr_to_pst = fdr_to_pst_holder.data ();
>    fdr_to_pst++;
>    {
>
>

Re: MIPS patch for crash during elf load

[Andrew Burgess]

* Lightning via Gdb-patches <gdb-patches@sourceware.org> [2021-11-23 09:01:44 -0800]:

>  You didn't miss anything subtle and I did fail to point out it is part of
> the ECOFF parsing. I wasn't 100% sure how everything was tied together and
> if swapping to the std::vector was acceptable so I tried to be clear enough
> about how the issue pops up and figured if there was a better patch it
> would be pointed out. Interestingly I don't see the issue show up on
> smaller binaries, just on larger ones where more memory has been reused
> hence my extra statement as I was trying to avoid the "I can't replicate"
> situation.

Thanks for checking this, and thank you for taking the time to track
down the cause of the bug.

I pushed my version of this patch.

Thanks again,
Andrew


> 
> I can confirm the patch you provided works just as well. Now I'm off to
> figure out why I'm getting abbrev 111 CU errors.




> 
> Jewell
> 
> On Tue, Nov 23, 2021 at 7:13 AM Andrew Burgess <aburgess@redhat.com> wrote:
> 
> > * Lightning via Gdb-patches <gdb-patches@sourceware.org> [2021-11-22
> > 20:50:03 -0800]:
> >
> > > GDB has a bug with MIPS binaries where loading ones with a large number
> > of
> > > global objects in the debug sections results in using uninitialized
> > memory
> > > resulting in a crash. This was seen during a debug build of the following
> > > project: https://github.com/pmret/papermario
> > >
> > > I downloaded GDB 11.1 and compiled it with the following options:
> > > ./configure --target=mips-linux-gnu --program-prefix=mips-linux-gnu-
> > >
> > > My local gcc compiler is gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> > >
> > > I traced the issue down to the initialization of the fdt_to_pst buffer.
> > It
> > > is initialized in gdb/mdebugread.c on line 2369 via a gdb::def_vector
> > > however the internal values are never cleared.  On line 2403 a struct
> > > variable in this buffer is incremented for the number of globals. With
> > > enough globals and sections this can result in having invalid data in the
> > > global field causing a crash. In the above case it was stale pointer
> > data.
> > > The fix is the attached patch, a simple memset after the buffer is
> > > initialized.
> >
> > Thanks for looking into this issue.
> >
> > I didn't really understand the back half of your problem description
> > here.
> >
> > I understand "On line 2403 a struct variable in this buffer is
> > incremented for the number of globals." and this looks bad, but then
> > you say, "With enough globals and sections this can result in having
> > invalid data in the global field causing a crash."
> >
> > I don't understand what the "enough globals" bit is about?
> >
> > As the gdb::def_vector doesn't initialize the backing memory, surely
> > we can just say something like: On line 2403 a struct variable in this
> > buffer is incremented for the number of globals, and as the memory was
> > not zero initialized the result of the increment is undefined.  This
> > can lead to undefined behaviour later on, which can result in a crash?
> >
> > Or have I missed some subtle detail of this issue?
> >
> > Also your subject line talks about mips and elf, but doesn't mention
> > the critical component here, ECOFF debug information, maybe: "fix crash
> > when reading ECOFF debug information" would be more on topic?
> >
> > On the fix itself, gdb::def_vector is a variation on std::vector
> > specifically designed NOT to initialize the backing memory.  Given we
> > are no immediately initializing the memory, I wonder if switching to
> > std::vector would be better?
> >
> > I brought all this together in the patch below.  As I don't have any
> > way to get this, I wonder if you'd be willing to give this patch a try
> > please,
> >
> > Thanks,
> > Andrew
> >
> > ----
> >
> > commit 7c3dae84e2051aff97dd1da1115c476a51072808
> > Author: Andrew Burgess <aburgess@redhat.com>
> > Date:   Mon Nov 22 20:52:15 2021 -0800
> >
> >     gdb: fix crash when reading ECOFF debug information
> >
> >     In commit:
> >
> >       commit 633cf2548bcd3dafe297e21a1dd3574240280d48
> >       Date:   Wed May 9 15:42:28 2018 -0600
> >
> >           Remove cleanups from mdebugread.c
> >
> >     the following change was made in the function parse_partial_symbols in
> >     mdebugread.c:
> >
> >       -  fdr_to_pst = XCNEWVEC (struct pst_map, hdr->ifdMax + 1);
> >       -  old_chain = make_cleanup (xfree, fdr_to_pst);
> >       +  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax +
> > 1);
> >       +  fdr_to_pst = fdr_to_pst_holder.data ();
> >
> >     The problem with this change is that XCNEWVEC calls xcalloc, which in
> >     turn calls calloc, and calloc zero initializes the allocated memory.
> >     In contrast, the new line gdb::def_vector<struct pst_map> specifically
> >     does not initialize the underlying memory.
> >
> >     This is a problem because, later on in this same function, we
> >     increment the n_globals field within 'struct pst_map' objects stored
> >     in the vector.  The incrementing is now being done from an
> >     uninitialized starting point.
> >
> >     In this commit we switch from using gdb::def_vector to using
> >     std::vector, this alone should be enough to ensure that the fields are
> >     initialized to zero.
> >
> >     However, for extra clarity, I have also added initial values in the
> >     'struct pst_map' to make it crystal clear how the struct will start
> >     up.
> >
> >     This issue was reported on the mailing list here:
> >
> >
> > https://sourceware.org/pipermail/gdb-patches/2021-November/183693.html
> >
> >     Co-Authored-By: Lightning <lightningth@gmail.com>
> >
> > diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
> > index 0faff5f43c8..9204d3debe6 100644
> > --- a/gdb/mdebugread.c
> > +++ b/gdb/mdebugread.c
> > @@ -386,9 +386,9 @@ mdebug_build_psymtabs (minimal_symbol_reader &reader,
> >
> >  struct pst_map
> >  {
> > -  legacy_psymtab *pst; /* the psymtab proper */
> > -  long n_globals;              /* exported globals (external symbols) */
> > -  long globals_offset;         /* cumulative */
> > +  legacy_psymtab *pst = nullptr;  /* the psymtab proper */
> > +  long n_globals = 0;            /* exported globals (external symbols) */
> > +  long globals_offset = 0;       /* cumulative */
> >  };
> >
> >
> > @@ -2365,7 +2365,7 @@ parse_partial_symbols (minimal_symbol_reader &reader,
> >    /* Allocate the map FDR -> PST.
> >       Minor hack: -O3 images might claim some global data belongs
> >       to FDR -1.  We`ll go along with that.  */
> > -  gdb::def_vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
> > +  std::vector<struct pst_map> fdr_to_pst_holder (hdr->ifdMax + 1);
> >    fdr_to_pst = fdr_to_pst_holder.data ();
> >    fdr_to_pst++;
> >    {
> >
> >

Re: MIPS patch for crash during elf load

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Thanks for checking this, and thank you for taking the time to track
Andrew> down the cause of the bug.

Thanks to both of you for fixing this.

Andrew> I pushed my version of this patch.

This seems like a candidate for 11.2.

Tom

Re: MIPS patch for crash during elf load

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-11-30 14:29:08 -0700]:

> >>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> Thanks for checking this, and thank you for taking the time to track
> Andrew> down the cause of the bug.
> 
> Thanks to both of you for fixing this.
> 
> Andrew> I pushed my version of this patch.
> 
> This seems like a candidate for 11.2.

Sure.  Do I need a bug number to back port patches, or do I just go
for it?

Thanks,
Andrew

Re: MIPS patch for crash during elf load

[Tom Tromey]

>> This seems like a candidate for 11.2.

Andrew> Sure.  Do I need a bug number to back port patches, or do I just go
Andrew> for it?

Yeah, a bug is needed.

Tom