From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A7B803858D28 for ; Tue, 23 Nov 2021 15:13:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A7B803858D28 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-320-9oX8AiD8NWapnbmr40Ui_w-1; Tue, 23 Nov 2021 10:12:57 -0500 X-MC-Unique: 9oX8AiD8NWapnbmr40Ui_w-1 Received: by mail-wr1-f69.google.com with SMTP id d7-20020a5d6447000000b00186a113463dso3780331wrw.10 for ; Tue, 23 Nov 2021 07:12:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=raVnPXF/1dDSQ1gx87ugyUfggt1/kECuPIw2+b0OU/c=; b=TrZaCFiL+WhIqdA4WlinJFj4/26ynNzpwPnMieG3Ih+zHk0KJUP8R9QoqO1y+1vGIa hkgkc8HHd9eHzsaWBvoJ+SxKuI2YmXFlZBdrqoO5IZMAcOpZ3sdDQDoLXqWDIipcpLsG ZV+3ymJL2Yf27LiroB3jcAKQ9ofboWy6NKHTQLWEWeyTPO+3AepETODhnDOOWCAOX3vQ Qnwb4snDSMZorRuS8DjbOnYwIVGeQ1+/+mqUa4FEB/WmQJs6nwgBvELSBgM7j0elVukw SXhVY1FtkVHDVj9X5y4dPGs3hGnwl28yquuXyWabAmf3Sz9Bd0VXQ2BXhnivDsJ0YK1y KlmA== X-Gm-Message-State: AOAM533aY0CZ/KcYR66H/yyxE97r//sklVq2z7lPw3VrwCd6tkOgfqUQ vqsmuo+d9NY5MDUGGFqFjhq38Zg9dMaLGDoBry4cibGtkHSltfZmjs1vOxjZP/k+Qego9lXeMlH uLBPH7b1ZEfEEAjtbS4km0Q== X-Received: by 2002:a5d:6c67:: with SMTP id r7mr8418810wrz.286.1637680376090; Tue, 23 Nov 2021 07:12:56 -0800 (PST) X-Google-Smtp-Source: ABdhPJwMTcxR3AH5afwY6HqtjNUCBjUz/J2lSAv+xLLfwAD6ntIQchKFHpxJMmRl4K68lFPHHUucHA== X-Received: by 2002:a5d:6c67:: with SMTP id r7mr8418781wrz.286.1637680375874; Tue, 23 Nov 2021 07:12:55 -0800 (PST) Received: from localhost (host86-166-129-255.range86-166.btcentralplus.com. [86.166.129.255]) by smtp.gmail.com with ESMTPSA id m17sm11944025wrz.22.2021.11.23.07.12.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Nov 2021 07:12:55 -0800 (PST) Date: Tue, 23 Nov 2021 15:12:54 +0000 From: Andrew Burgess To: Lightning Cc: gdb-patches@sourceware.org Subject: Re: MIPS patch for crash during elf load Message-ID: <20211123151254.GL2514@redhat.com> References: MIME-Version: 1.0 In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 14:42:45 up 4 days, 3:41, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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 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 15:13:02 -0000 * 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++; {