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

[Andrew Burgess]

* 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;