gdb/opcodes: avoid crash when architecture is forced to csky or riscv

Message ID 20210519144912.1707118-1-andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb/opcodes: avoid crash when architecture is forced to csky or riscv
Related show

Commit Message

Andrew Burgess May 19, 2021, 2:49 p.m.
I built GDB with `--enable-targets=all`, then started GDB passing it
an x86-64 executable, finally I ran 'maint selftest', and observed GDB
crash like this:

  BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
  Aborted (core dumped)

The problem originates from two locations, for example in csky-dis.c
(csky_get_disassembler) where we do this:

  const char *sec_name = NULL;
  ...
  sec_name = get_elf_backend_data (abfd)->obj_attrs_section;
  if (bfd_get_section_by_name (abfd, sec_name) != NULL)
    ...

We end up in here because during the selftests GDB forces the
architecture to be csky, but the BFD being accessed is still of type
x86-64.  As a result obj_attrs_section returns NULL, which means we
end up passing NULL to bfd_get_section_by_name.  If we follow the
function calls from bfd_get_section_by_name we eventually end up in
bfd_hash_hash, which asserts that the string (i.e. the name) is not
NULL.

The same crash can be reproduced in GDB without using the selftests,
for example:

  (gdb) file x86_64.elf
  (gdb) start
  (gdb) set architecture csky
  (gdb) disassemble main
  Dump of assembler code for function main:
  BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
  Aborted (core dumped)

The fix I propose here is to check if obj_attrs_section is NULL before
calling bfd_get_section_by_name.

opcodes/ChangeLog:

	* csky-dis.c (csky_get_disassembler): Handle no obj_attrs_section.
	* riscv-dis.c (riscv_get_disassembler): Likewise.
---
 opcodes/ChangeLog   | 5 +++++
 opcodes/csky-dis.c  | 3 ++-
 opcodes/riscv-dis.c | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.25.4

Comments

H.J. Lu via Binutils May 19, 2021, 4 p.m. | #1
Hi Andrew

I would suggest to check sec_name for NULL inside of
bfd_get_section_by_name function.

Best regards,
Alex

On Wed, 19 May 2021 at 18:36, Andrew Burgess <andrew.burgess@embecosm.com>
wrote:

> I built GDB with `--enable-targets=all`, then started GDB passing it

> an x86-64 executable, finally I ran 'maint selftest', and observed GDB

> crash like this:

>

>   BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail

> ../../src/bfd/hash.c:438

>   Aborted (core dumped)

>

> The problem originates from two locations, for example in csky-dis.c

> (csky_get_disassembler) where we do this:

>

>   const char *sec_name = NULL;

>   ...

>   sec_name = get_elf_backend_data (abfd)->obj_attrs_section;

>   if (bfd_get_section_by_name (abfd, sec_name) != NULL)

>     ...

>

> We end up in here because during the selftests GDB forces the

> architecture to be csky, but the BFD being accessed is still of type

> x86-64.  As a result obj_attrs_section returns NULL, which means we

> end up passing NULL to bfd_get_section_by_name.  If we follow the

> function calls from bfd_get_section_by_name we eventually end up in

> bfd_hash_hash, which asserts that the string (i.e. the name) is not

> NULL.

>

> The same crash can be reproduced in GDB without using the selftests,

> for example:

>

>   (gdb) file x86_64.elf

>   (gdb) start

>   (gdb) set architecture csky

>   (gdb) disassemble main

>   Dump of assembler code for function main:

>   BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail

> ../../src/bfd/hash.c:438

>   Aborted (core dumped)

>

> The fix I propose here is to check if obj_attrs_section is NULL before

> calling bfd_get_section_by_name.

>

> opcodes/ChangeLog:

>

>         * csky-dis.c (csky_get_disassembler): Handle no obj_attrs_section.

>         * riscv-dis.c (riscv_get_disassembler): Likewise.

> ---

>  opcodes/ChangeLog   | 5 +++++

>  opcodes/csky-dis.c  | 3 ++-

>  opcodes/riscv-dis.c | 3 ++-

>  3 files changed, 9 insertions(+), 2 deletions(-)

>

> diff --git a/opcodes/csky-dis.c b/opcodes/csky-dis.c

> index cdd911be09a..f78bbe853f6 100644

> --- a/opcodes/csky-dis.c

> +++ b/opcodes/csky-dis.c

> @@ -249,7 +249,8 @@ csky_get_disassembler (bfd *abfd)

>        /* Skip any input that hasn't attribute section.

>           This enables to link object files without attribute section with

>           any others.  */

> -      if (bfd_get_section_by_name (abfd, sec_name) != NULL)

> +      if (sec_name != NULL

> +          && bfd_get_section_by_name (abfd, sec_name) != NULL)

>          {

>            attr = elf_known_obj_attributes_proc (abfd);

>            dis_info.isa = attr[Tag_CSKY_ISA_EXT_FLAGS].i;

> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c

> index fe8dfb88d90..cf64963a7c3 100644

> --- a/opcodes/riscv-dis.c

> +++ b/opcodes/riscv-dis.c

> @@ -601,7 +601,8 @@ riscv_get_disassembler (bfd *abfd)

>        if (ebd)

>          {

>           const char *sec_name = ebd->obj_attrs_section;

> -         if (bfd_get_section_by_name (abfd, sec_name) != NULL)

> +         if (sec_name != NULL

> +              && bfd_get_section_by_name (abfd, sec_name) != NULL)

>             {

>               obj_attribute *attr = elf_known_obj_attributes_proc (abfd);

>               unsigned int Tag_a = Tag_RISCV_priv_spec;

> --

> 2.25.4

>

>
H.J. Lu via Binutils May 20, 2021, 3:17 a.m. | #2
On Wed, May 19, 2021 at 03:49:12PM +0100, Andrew Burgess wrote:
> The fix I propose here is to check if obj_attrs_section is NULL before

> calling bfd_get_section_by_name.

> 

> opcodes/ChangeLog:

> 

> 	* csky-dis.c (csky_get_disassembler): Handle no obj_attrs_section.

> 	* riscv-dis.c (riscv_get_disassembler): Likewise.


OK.

-- 
Alan Modra
Australia Development Lab, IBM
H.J. Lu via Binutils May 20, 2021, 3:23 a.m. | #3
On Wed, May 19, 2021 at 07:00:14PM +0300, Alexander Fedotov via Binutils wrote:
> Hi Andrew

> 

> I would suggest to check sec_name for NULL inside of

> bfd_get_section_by_name function.


That suggestion has some merit.  If you, Alexander, would like to
implement it and remove all the existing NULL name checks that would
then be redundant, I'll approve such a patch.

-- 
Alan Modra
Australia Development Lab, IBM
Andrew Burgess May 20, 2021, 8:25 a.m. | #4
* Alan Modra <amodra@gmail.com> [2021-05-20 12:53:13 +0930]:

> On Wed, May 19, 2021 at 07:00:14PM +0300, Alexander Fedotov via Binutils wrote:

> > Hi Andrew

> > 

> > I would suggest to check sec_name for NULL inside of

> > bfd_get_section_by_name function.

> 

> That suggestion has some merit.  If you, Alexander, would like to

> implement it and remove all the existing NULL name checks that would

> then be redundant, I'll approve such a patch.


Thanks for the feedback.

The patch below moves the NULL check into bfd_get_section_by_name.  I
searched through looking for unneeded NULL checks that could be
removed, and didn't find any (though I could have missed some).

Would this be better than my original suggestion?

Thanks,
Andrew

---

commit 2c26a646ef722c131b605c2aad34a9d3f66ff1b9
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu May 20 09:16:41 2021 +0100

    gdb/bfd: avoid crash when architecture is forced to csky or riscv
    
    I built GDB with `--enable-targets=all`, then started GDB passing it
    an x86-64 executable, finally I ran 'maint selftest', and observed GDB
    crash like this:
    
      BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
      Aborted (core dumped)
    
    The problem originates from two locations, for example in csky-dis.c
    (csky_get_disassembler) where we do this:
    
      const char *sec_name = NULL;
      ...
      sec_name = get_elf_backend_data (abfd)->obj_attrs_section;
      if (bfd_get_section_by_name (abfd, sec_name) != NULL)
        ...
    
    We end up in here because during the selftests GDB forces the
    architecture to be csky, but the BFD being accessed is still of type
    x86-64.  As a result obj_attrs_section returns NULL, which means we
    end up passing NULL to bfd_get_section_by_name.  If we follow the
    function calls from bfd_get_section_by_name we eventually end up in
    bfd_hash_hash, which asserts that the string (i.e. the name) is not
    NULL.
    
    The same crash can be reproduced in GDB without using the selftests,
    for example:
    
      (gdb) file x86_64.elf
      (gdb) start
      (gdb) set architecture csky
      (gdb) disassemble main
      Dump of assembler code for function main:
      BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
      Aborted (core dumped)
    
    The fix I propose here is to have bfd_get_section_by_name return NULL
    if name is ever NULL.  For consistency I updated
    bfd_get_section_by_name_if in the same way, even though I'm not
    hitting any problems along that code path right now.
    
    I looked through the source tree to see if I could find anywhere that
    we check name for NULL before calling these functions (as these checks
    would now be redundant), but couldn't find any - though it's quite
    possible I've missed some.
    
    bfd/ChangeLog:
    
            * section.c (bfd_get_section_by_name): Return NULL if name is
            NULL.
            (bfd_get_section_by_name_if): Likewise.

diff --git a/bfd/section.c b/bfd/section.c
index a35348833d9..6b6aa92b968 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -898,6 +898,9 @@ bfd_get_section_by_name (bfd *abfd, const char *name)
 {
   struct section_hash_entry *sh;
 
+  if (name == NULL)
+    return NULL;
+
   sh = section_hash_lookup (&abfd->section_htab, name, false, false);
   if (sh != NULL)
     return &sh->section;
@@ -1006,6 +1009,9 @@ bfd_get_section_by_name_if (bfd *abfd, const char *name,
   struct section_hash_entry *sh;
   unsigned long hash;
 
+  if (name == NULL)
+    return NULL;
+
   sh = section_hash_lookup (&abfd->section_htab, name, false, false);
   if (sh == NULL)
     return NULL;
H.J. Lu via Binutils May 20, 2021, 9:12 a.m. | #5
On Thu, May 20, 2021 at 09:25:13AM +0100, Andrew Burgess wrote:
> The patch below moves the NULL check into bfd_get_section_by_name.  I

> searched through looking for unneeded NULL checks that could be

> removed, and didn't find any (though I could have missed some).


There are two in bfd/dwarf2.c

> Would this be better than my original suggestion?


A little bit, yes.  Thanks, OK with or without the dwarf2.c changes
too.

-- 
Alan Modra
Australia Development Lab, IBM
Andrew Burgess May 20, 2021, 12:45 p.m. | #6
* Alan Modra <amodra@gmail.com> [2021-05-20 18:42:35 +0930]:

> On Thu, May 20, 2021 at 09:25:13AM +0100, Andrew Burgess wrote:

> > The patch below moves the NULL check into bfd_get_section_by_name.  I

> > searched through looking for unneeded NULL checks that could be

> > removed, and didn't find any (though I could have missed some).

> 

> There are two in bfd/dwarf2.c

> 

> > Would this be better than my original suggestion?

> 

> A little bit, yes.  Thanks, OK with or without the dwarf2.c changes

> too.


Thanks for the feedback, I made the bfd/dwarf2.c changes.  Below is
what I pushed.

Thanks,
Andrew

---

commit 427e4066afd13d1bf52c849849475f536e285d66
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu May 20 09:16:41 2021 +0100

    gdb/bfd: avoid crash when architecture is forced to csky or riscv
    
    I built GDB with `--enable-targets=all`, then started GDB passing it
    an x86-64 executable, finally I ran 'maint selftest', and observed GDB
    crash like this:
    
      BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
      Aborted (core dumped)
    
    The problem originates from two locations, for example in csky-dis.c
    (csky_get_disassembler) where we do this:
    
      const char *sec_name = NULL;
      ...
      sec_name = get_elf_backend_data (abfd)->obj_attrs_section;
      if (bfd_get_section_by_name (abfd, sec_name) != NULL)
        ...
    
    We end up in here because during the selftests GDB forces the
    architecture to be csky, but the BFD being accessed is still of type
    x86-64.  As a result obj_attrs_section returns NULL, which means we
    end up passing NULL to bfd_get_section_by_name.  If we follow the
    function calls from bfd_get_section_by_name we eventually end up in
    bfd_hash_hash, which asserts that the string (i.e. the name) is not
    NULL.
    
    The same crash can be reproduced in GDB without using the selftests,
    for example:
    
      (gdb) file x86_64.elf
      (gdb) start
      (gdb) set architecture csky
      (gdb) disassemble main
      Dump of assembler code for function main:
      BFD: BFD (GNU Binutils) 2.36.50.20210519 assertion fail ../../src/bfd/hash.c:438
      Aborted (core dumped)
    
    The fix I propose here is to have bfd_get_section_by_name return NULL
    if name is ever NULL.  For consistency I updated
    bfd_get_section_by_name_if in the same way, even though I'm not
    hitting any problems along that code path right now.
    
    I looked through the source tree and removed two NULL checks in
    bfd/dwarf2.c which are no longer needed, its possible that there are
    additional NULL checks that could be removed, I just didn't find them.
    
    bfd/ChangeLog:
    
            * section.c (bfd_get_section_by_name): Return NULL if name is
            NULL.
            (bfd_get_section_by_name_if): Likewise.
            * dwarf2.c (read_section): Remove unneeded NULL check.
            (find_debug_info): Likewise.

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index 0a8a5578da8..cf1f1d1f1ff 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -545,8 +545,7 @@ read_section (bfd *	      abfd,
       if (msec == NULL)
 	{
 	  section_name = sec->compressed_name;
-	  if (section_name != NULL)
-	    msec = bfd_get_section_by_name (abfd, section_name);
+          msec = bfd_get_section_by_name (abfd, section_name);
 	}
       if (msec == NULL)
 	{
@@ -4226,12 +4225,9 @@ find_debug_info (bfd *abfd, const struct dwarf_debug_section *debug_sections,
 	return msec;
 
       look = debug_sections[debug_info].compressed_name;
-      if (look != NULL)
-	{
-	  msec = bfd_get_section_by_name (abfd, look);
-	  if (msec != NULL)
-	    return msec;
-	}
+      msec = bfd_get_section_by_name (abfd, look);
+      if (msec != NULL)
+        return msec;
 
       for (msec = abfd->sections; msec != NULL; msec = msec->next)
 	if (startswith (msec->name, GNU_LINKONCE_INFO))
diff --git a/bfd/section.c b/bfd/section.c
index a35348833d9..6b6aa92b968 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -898,6 +898,9 @@ bfd_get_section_by_name (bfd *abfd, const char *name)
 {
   struct section_hash_entry *sh;
 
+  if (name == NULL)
+    return NULL;
+
   sh = section_hash_lookup (&abfd->section_htab, name, false, false);
   if (sh != NULL)
     return &sh->section;
@@ -1006,6 +1009,9 @@ bfd_get_section_by_name_if (bfd *abfd, const char *name,
   struct section_hash_entry *sh;
   unsigned long hash;
 
+  if (name == NULL)
+    return NULL;
+
   sh = section_hash_lookup (&abfd->section_htab, name, false, false);
   if (sh == NULL)
     return NULL;

Patch

diff --git a/opcodes/csky-dis.c b/opcodes/csky-dis.c
index cdd911be09a..f78bbe853f6 100644
--- a/opcodes/csky-dis.c
+++ b/opcodes/csky-dis.c
@@ -249,7 +249,8 @@  csky_get_disassembler (bfd *abfd)
       /* Skip any input that hasn't attribute section.
          This enables to link object files without attribute section with
          any others.  */
-      if (bfd_get_section_by_name (abfd, sec_name) != NULL)
+      if (sec_name != NULL
+          && bfd_get_section_by_name (abfd, sec_name) != NULL)
         {
           attr = elf_known_obj_attributes_proc (abfd);
           dis_info.isa = attr[Tag_CSKY_ISA_EXT_FLAGS].i;
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index fe8dfb88d90..cf64963a7c3 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -601,7 +601,8 @@  riscv_get_disassembler (bfd *abfd)
       if (ebd)
         {
 	  const char *sec_name = ebd->obj_attrs_section;
-	  if (bfd_get_section_by_name (abfd, sec_name) != NULL)
+	  if (sec_name != NULL
+              && bfd_get_section_by_name (abfd, sec_name) != NULL)
 	    {
 	      obj_attribute *attr = elf_known_obj_attributes_proc (abfd);
 	      unsigned int Tag_a = Tag_RISCV_priv_spec;