Check for null result from gdb_demangle

  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(+)
  

* 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