diff mbox

Check for null result from gdb_demangle

Message ID 20200213063140.129700-1-tamur@google.com
State New
Headers show

Commit Message

Doug Evans 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

Andrew Burgess Feb. 13, 2020, 10:52 a.m. UTC | #1
* 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
>
Simon Marchi Feb. 13, 2020, 2:57 p.m. UTC | #2
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
Pedro Alves Feb. 13, 2020, 6:48 p.m. UTC | #3
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;
>  
>
Doug Evans via gdb-patches Feb. 13, 2020, 7:20 p.m. UTC | #4
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)?
Andrew Burgess Feb. 18, 2020, 10:39 a.m. UTC | #5
* 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
Simon Marchi Feb. 18, 2020, 7:54 p.m. UTC | #6
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?
Tom Tromey Feb. 21, 2020, 3:19 p.m. UTC | #7
>>>>> "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
Andrew Burgess Feb. 21, 2020, 3:36 p.m. UTC | #8
* 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 mbox

Patch

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;