Don't return "(null)" from bfd_elf_sym_name

[Thiago Jung Bauermann]

Alan Modra <amodra@gmail.com> writes:

> On Thu, Oct 03, 2024 at 09:45:56PM -0300, Thiago Jung Bauermann wrote:
>> Hello,
>> 
>> Alan Modra <amodra@gmail.com> writes:
>> 
>> > A NULL return from bfd_elf_string_from_elf_section indicates an error.
>> > That shouldn't be masked by bfd_elf_sym_name but rather passed up to
>> > callers such as group_signature.  If we want to print "(null)" then
>> > that should be done at a higher level.  That's what this patch does,
>> > except that I chose to print "<null>" instead, like readelf.  If we
>> > see "(null)" we're probably passing a NULL to printf.  I haven't
>> > changed aoutx.h or pdp11.c print_symbol functions because they already
>> > handle NULL names by omitting the name.  I also haven't changed
>> > mach-o.c, mmo.c, som.c, srec.c, tekhex.c, vms-alpha.c and
>> > wasm-module.c print_symbol function because it looks like they will
>> > never have NULL symbol names.
>> 
>> This patch causes GDB to segfault in a test with a mangled symbol table:
>> 
>> Running gdb.base/bfd-errors.exp ...
>> ERROR: GDB process no longer exists
>> UNRESOLVED: gdb.base/bfd-errors.exp: load library with add-symbol-file
>
> Yes, I was just looking at this and had written the following:
>
> From a88c23dda6360e247fa758f3482ee7cf661a29cf Mon Sep 17 00:00:00 2001
> From: Alan Modra <amodra@gmail.com>
> Date: Fri, 4 Oct 2024 07:47:05 +0930
> Subject: gdb segv in elfread.c:elf_rel_plt_read
>
> After commit 68bbe1183379, ELF symbols read via bfd_canonicalize_symtab
> and similar functions which have bad st_name fields will have NULL in
> the name rather than "(null)".  gdb.base/bfd-errors.exp deliberately
> creates a faulty shared library with st_name pointing outside of
> .dynsym for two symbols, and thus now results in NULL symbol names.
> This triggers a segv on string_buffer.assign(name).  Fix that.
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index e959d3a2f9d..2e68b0dba1a 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -612,6 +612,8 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>        const size_t got_suffix_len = strlen (SYMBOL_GOT_PLT_SUFFIX);
>  
>        name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
> +      if (!name)
> +	continue;
>        address = relplt->relocation[reloc].address;
>  
>        asection *msym_section;
>
> I think it's better to ignore symbols with NULL names rather than turn
> the name into "(null)" as you do in your patch, since you can't really
> do anything sensible when looking up "(null)@got.plt" later.

I agree that makes more sense. Thank you for investigating and fixing
the problem. Will you submit the patch to GDB?

-- 
Thiago