Message ID | 20200213063140.129700-1-tamur@google.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 96578 invoked by alias); 13 Feb 2020 06:31:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 96570 invoked by uid 89); 13 Feb 2020 06:31:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-pf1-f202.google.com Received: from mail-pf1-f202.google.com (HELO mail-pf1-f202.google.com) (209.85.210.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Feb 2020 06:31:45 +0000 Received: by mail-pf1-f202.google.com with SMTP id o1so3126142pfg.6 for <gdb-patches@sourceware.org>; Wed, 12 Feb 2020 22:31:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=n/Pqmv1dEY3tadTTskNeAkmC/+iyUODcOfuSR3nlzAM=; b=AVqneMt0kAKOCzydNoJq4HQU440SJfb0Bs46PQvljkCSFXZmv9sURbY7ZVxHZrPFtQ Y7S+orKGk6QgK/lWfJiT/dgP16MGG8hEaFTK6cS6dO8T3o3lO5IAgFL0pDt15xuza0Rw 4QhdXkfPT4cRWf5+saM1jJFVkeSnRLM5jv8Ti8/H1pYO+DxxOq1w+fl3mDNekljl14hO 0htX0Zg6MLJl5wTzQCwG0G/LXgMoJstpvKm0az6Kk7K7MG5whoQnSizGdzI6gLMDIMH6 owNlLYeSoNSyR6/ZcZ8oru0CvipHOcQpyolccXipR/VXrHIYByGZeJzVWpz5EwagqWbv qkGQ== Date: Wed, 12 Feb 2020 22:31:40 -0800 Message-Id: <20200213063140.129700-1-tamur@google.com> Mime-Version: 1.0 Subject: [PATCH] Check for null result from gdb_demangle From: "Ali Tamur via gdb-patches" <gdb-patches@sourceware.org> Reply-To: Ali Tamur <tamur@google.com> To: gdb-patches@sourceware.org Cc: kmoy@google.com, Ali Tamur <tamur@google.com> Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes |
Commit Message
Terekhov, Mikhail via Gdb-patches
Feb. 13, 2020, 6:31 a.m. UTC
I am sending this patch on behalf of kmoy@google.com, who discovered the bug and wrote the fix. gdb_demangle can return null for strings that don't properly demangle. The null check was mistakenly removed in commit 43816ebc335. Without this check, GDB aborts when loading symbols from some binaries. gdb/ChangeLog: * dwarf2/read.c (dwarf2_name): Add null check. --- gdb/dwarf2/read.c | 2 ++ 1 file changed, 2 insertions(+)
Comments
* Ali Tamur via gdb-patches <gdb-patches@sourceware.org> [2020-02-12 22:31:40 -0800]: > I am sending this patch on behalf of kmoy@google.com, who discovered the bug > and wrote the fix. > > gdb_demangle can return null for strings that don't properly demangle. The null > check was mistakenly removed in commit 43816ebc335. Without this check, GDB > aborts when loading symbols from some binaries. > > gdb/ChangeLog: > > * dwarf2/read.c (dwarf2_name): Add null check. If you are able to find an example of a symbol that triggers the crash then it should be pretty easy to add a test, see for example the last few lines of gdb.cp/demangle.exp for C++ demangling tests. Having a test would help something like this happening again. Thanks, Andrew > --- > gdb/dwarf2/read.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 7edbd9d7df..2f37c8a496 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -21770,6 +21770,8 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu) > { > gdb::unique_xmalloc_ptr<char> demangled > (gdb_demangle (DW_STRING (attr), DMGL_TYPES)); > + if (demangled == nullptr) > + return nullptr; > > const char *base; > > -- > 2.25.0.265.gbab2e86ba0-goog >
On 2020-02-13 5:52 a.m., Andrew Burgess wrote: > If you are able to find an example of a symbol that triggers the crash > then it should be pretty easy to add a test, see for example the last > few lines of gdb.cp/demangle.exp for C++ demangling tests. Having a > test would help something like this happening again. Calling the "demangle" command doesn't seem to invoke that function though. Simon
On 2/13/20 6:31 AM, Ali Tamur via gdb-patches wrote: > I am sending this patch on behalf of kmoy@google.com, who discovered the bug > and wrote the fix. Please make sure that his name is used in the ChangeLog entry as well as git authorship. (When the latter is done, git format-patch/send-email put a "From: ..." as the first line of the email, which is missing here, which makes me suspect authorship isn't correct.) Thanks, Pedro Alves > > gdb_demangle can return null for strings that don't properly demangle. The null > check was mistakenly removed in commit 43816ebc335. Without this check, GDB > aborts when loading symbols from some binaries. > > gdb/ChangeLog: > > * dwarf2/read.c (dwarf2_name): Add null check. > --- > gdb/dwarf2/read.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 7edbd9d7df..2f37c8a496 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -21770,6 +21770,8 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu) > { > gdb::unique_xmalloc_ptr<char> demangled > (gdb_demangle (DW_STRING (attr), DMGL_TYPES)); > + if (demangled == nullptr) > + return nullptr; > > const char *base; > >
On Thu, Feb 13, 2020 at 2:52 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > If you are able to find an example of a symbol that triggers the crash > The symbol where I ran into the problem was "<anon>". I see references to this in GCC sources, but I'm not entirely clear on when this is emitted instead of "<anonymous struct>", "<anonymous union>", "<anonymous>", or similar. then it should be pretty easy to add a test, see for example the last > few lines of gdb.cp/demangle.exp for C++ demangling tests. Having a > test would help something like this happening again. > Given that the bug here is that this code doesn't check for the possibility that gdb_demangle can fail (not that it failed), what kind of test would you recommend (and where)?
* Keith Moyer <kmoy@google.com> [2020-02-13 11:20:46 -0800]: > On Thu, Feb 13, 2020 at 2:52 AM Andrew Burgess <andrew.burgess@embecosm.com> > wrote: > > > If you are able to find an example of a symbol that triggers the crash > > > > The symbol where I ran into the problem was "<anon>". I see references to > this in GCC sources, but I'm not entirely clear on when this is emitted > instead of "<anonymous struct>", "<anonymous union>", "<anonymous>", or > similar. > > then it should be pretty easy to add a test, see for example the last > > few lines of gdb.cp/demangle.exp for C++ demangling tests. Having a > > test would help something like this happening again. > > > > Given that the bug here is that this code doesn't check for the possibility > that gdb_demangle can fail (not that it failed), what kind of test would > you recommend (and where)? You're absolutely right. Sorry for the confusion. I withdraw my request. Thanks, Andrew
On 2020-02-18 5:39 a.m., Andrew Burgess wrote: > * Keith Moyer <kmoy@google.com> [2020-02-13 11:20:46 -0800]: > >> On Thu, Feb 13, 2020 at 2:52 AM Andrew Burgess <andrew.burgess@embecosm.com> >> wrote: >> >>> If you are able to find an example of a symbol that triggers the crash >>> >> >> The symbol where I ran into the problem was "<anon>". I see references to >> this in GCC sources, but I'm not entirely clear on when this is emitted >> instead of "<anonymous struct>", "<anonymous union>", "<anonymous>", or >> similar. >> >> then it should be pretty easy to add a test, see for example the last >>> few lines of gdb.cp/demangle.exp for C++ demangling tests. Having a >>> test would help something like this happening again. >>> >> >> Given that the bug here is that this code doesn't check for the possibility >> that gdb_demangle can fail (not that it failed), what kind of test would >> you recommend (and where)? > > You're absolutely right. Sorry for the confusion. I withdraw my > request. I might be missing something, but shouldn't it be possible to write a test in gdb.dwarf2 that creates a struct with linkage name <anon> (or any undemanglable identifier) and exercise this?
>>>>> "Ali" == Ali Tamur via gdb-patches <gdb-patches@sourceware.org> writes:
Ali> I am sending this patch on behalf of kmoy@google.com, who discovered the bug
Ali> and wrote the fix.
Ali> gdb_demangle can return null for strings that don't properly demangle. The null
Ali> check was mistakenly removed in commit 43816ebc335. Without this check, GDB
Ali> aborts when loading symbols from some binaries.
Ali> gdb/ChangeLog:
Ali> * dwarf2/read.c (dwarf2_name): Add null check.
I'm pushing this now. It came up a second time so I think it ought to
go in.
Tom
* Tom Tromey <tom@tromey.com> [2020-02-21 08:19:45 -0700]: > >>>>> "Ali" == Ali Tamur via gdb-patches <gdb-patches@sourceware.org> writes: > > Ali> I am sending this patch on behalf of kmoy@google.com, who discovered the bug > Ali> and wrote the fix. > > Ali> gdb_demangle can return null for strings that don't properly demangle. The null > Ali> check was mistakenly removed in commit 43816ebc335. Without this check, GDB > Ali> aborts when loading symbols from some binaries. > > Ali> gdb/ChangeLog: > > Ali> * dwarf2/read.c (dwarf2_name): Add null check. > > I'm pushing this now. It came up a second time so I think it ought to > go in. I pushed the test from this mail: https://sourceware.org/ml/gdb-patches/2020-02/msg00777.html Thanks, Andrew
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 7edbd9d7df..2f37c8a496 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -21770,6 +21770,8 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu) { gdb::unique_xmalloc_ptr<char> demangled (gdb_demangle (DW_STRING (attr), DMGL_TYPES)); + if (demangled == nullptr) + return nullptr; const char *base;