Patchwork Check for null result from gdb_demangle

login
register
mail settings
Submitter Doug Evans via gdb-patches
Date Feb. 13, 2020, 6:31 a.m.
Message ID <20200213063140.129700-1-tamur@google.com>
Download mbox | patch
Permalink /patch/38019/
State New
Headers show

Comments

Doug Evans via gdb-patches - Feb. 13, 2020, 6:31 a.m.
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(+)
Andrew Burgess - Feb. 13, 2020, 10:52 a.m.
* 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.
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.
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.
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)?

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;