From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com [IPv6:2607:f8b0:4864:20::72d]) by sourceware.org (Postfix) with ESMTPS id 207413858D28 for ; Tue, 23 Nov 2021 17:01:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 207413858D28 Received: by mail-qk1-x72d.google.com with SMTP id q64so22571665qkd.5 for ; Tue, 23 Nov 2021 09:01:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mmKasLr2kLcLeqlvE2tcYELlUhh2Sr7sPX/xez1EQYs=; b=SZSnX+Lt01wySWoU+m9onTmjTjAdFlzJAe7EfpSSXrwqiSwDIpvEIc6MFy79xxBRFC i7bf7ppB6ZrogrnEDQEpwG6Kn/XQ4hDMS7srggo1bsoNtrbUpG8S81+ffohjULEL09Vx YCHR6f2cHqPOu0MzjGSAz2nZmfkwAoFnT5v/yywg5WZDFpMht9terpLjDkC/RwDkI17u lbe1sZG8ad99PrkWme5iJmKBgwcMBoZq3FE8Py46sN7nzc2c+oSjnauBci2lHPcVfQFb +V92EMr8b6W4M6Tm/vwm7OxxGwOc4Em8c5dpo6iA1Cf2dcRdmL1Lhq82lkkb+5XE6hSw SClA== X-Gm-Message-State: AOAM532TlHiU5wEK2nVnReDBXX/Pcnm4HglwGfFLbN2xPn+h/pghT2TZ /rMCx8z2uUSofeRQZeCMY9CoL4uzPkJSdFhjbLKCa1aJqgM= X-Google-Smtp-Source: ABdhPJwtiBTvxpOYNcGtH9NxkM148TJ2GtdaHhMLhRbRWj0otXPhDBZ6BHgpOBbn/FmSwo8t5hp4c6f9U2XTquONH+g= X-Received: by 2002:a05:620a:cd0:: with SMTP id b16mr5882431qkj.401.1637686914729; Tue, 23 Nov 2021 09:01:54 -0800 (PST) MIME-Version: 1.0 References: <20211123151254.GL2514@redhat.com> In-Reply-To: <20211123151254.GL2514@redhat.com> From: Lightning Date: Tue, 23 Nov 2021 09:01:44 -0800 Message-ID: Subject: Re: MIPS patch for crash during elf load To: aburgess@redhat.com Cc: gdb-patches@sourceware.org X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Nov 2021 17:01:57 -0000 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 wrote: > * Lightning via Gdb-patches [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 > 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 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 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 > > 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 fdr_to_pst_holder (hdr->ifdMax + 1); > + std::vector fdr_to_pst_holder (hdr->ifdMax + 1); > fdr_to_pst = fdr_to_pst_holder.data (); > fdr_to_pst++; > { > >