MIPS patch for crash during elf load
Andrew Burgess
aburgess@redhat.com
Wed Nov 24 10:47:03 GMT 2021
* 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++;
> > {
> >
> >
More information about the Gdb-patches
mailing list