Re: [PATCH] Core file build-ids

[Keith Seitz <keiths at redhat dot com>]

Hi,

Has anyone had an opportunity to (or plan to) review?

Keith

On 10/1/19 6:07 AM, Keith Seitz wrote:
> Hi, Nick,
> 
> Wowser, it's been a busy quarter! I'm anxious to get back to this patch.
> I appreciate your patience.
> 
> On 6/7/19 9:16 AM, Nick Clifton wrote:
>>> +void
>>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
>>
>> I would recommend being a little bit paranoid here and checking
>> that templ is an elf format bfd before attempting to access the
>> backend data.
> 
> Done.
> 
>>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>>> +	return FALSE;
>>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>>> +      return TRUE;
>>
>> If _bfd_elf_core_find_build_id can fail (see above and below) then 
>> it would be worth checking the return value for an error result.
> 
> My concern here is what happens if we fail to find a build-id in the
> core file (I/O error jumping through a header or it's just plain missing).
> If _bfd_elf_core_find_build_id returns FALSE and we bail here, this will
> abort constructing any more phdrs.
> 
> After implementing your recommendation, I can actually demonstrate
> this scenario with one of gdb's tests: gdb.arch/i386-biarch-core.exp. This
> test will FAIL with a memory read error while attempting to print the
> contents of some memory location in the core file.
> 
> This location exists in a section that is internalized after we attempt
> to find a (missing) build-id.
> 
> That's why I made this an optional (as in, we don't care if it fails)
> step when constructing the section.
> 
> Maybe I've misunderstood what you were really asking me to do?
> 
>>> +/* Attempt to find a build-id in a core file from the core file BFD.
>>> +   OFFSET is the file offset to a PT_LOAD segment that may contain
>>> +   the build-id note.  */
>>> +
>>> +void
>>> +NAME(_bfd_elf,core_find_build_id)
>>
>> Given that things can wrong when attempting to find a build_id, I think
>> that this function should return a bfd_boolean.  I appreciate that the
>> caller could just check abfd->build_id != NULL, (assuming that it was NULL
>> to start with), but I think that it is cleaner that the function itself
>> should tell its caller whether ot not it succeeded.
> 
> I've made this change in this version, but at the one place it is called,
> the return value is ignored (see above). Nonetheless, I could imagine a
> use for this outside of my/gdb's specific use case, and I think returning
> bfd_boolean is a better/safer/more-forward-thinking option, so I've left
> it (updated per your request) in this version.
> 
>>> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
>>> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
>>
>> Minor formatting nit: comments should end with a full stop and two spaces.
> 
> Fixed.
> 
> New version below.
> 
> Keith
> 
> Core file build-ids
> 
> This patch adds support for reading build-ids from core files.  I've
> opted to maintain the current interfaces that BFD and GDB already use.
> 
> The approach taken is fairly straightforward (YMMV). When a core file
> is opened, BFD loops over the various segments in the ELF file and
> program headers. During this time, whenever a segment whose type is
> PT_LOAD is encountered, and the build-id is unknown, I've added a call
> to inspect the segment for an ELF header.  If a valid ELF file header
> is found, it attempts to read in the program headers and scan for any
> notes.
> 
> For those unfamiliar with build-ids, please see
> https://fedoraproject.org/wiki/Releases/FeatureBuildId .
> 
> Yes, this feature has been in Fedora GDB for over a decade, but it exists
> as a copy-n-paste of a lot of BFD's internals. For the brave, you can
> read the original submission at
> https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .
> 
> This is an attempt to clean that up.
> 
> I have run this patch through GDB's buildbot, and it has detected
> no problems.
> 
> bfd/ChangeLog:
> 
> 	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
> 	field.
> 	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
> 	functions.
> 	(elf_read_notes): Add declaration.
> 	* elf.c (elf_read_notes): Move elf-bfd.h.
> 	(_bfd_elf_core_find_build_id): New function.
> 	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
> 	build-id if none is known.
> 	(elf_parse_notes): For core files, scan for notes.
> 	* elfcore.h (elf_core_file_matches_executable_p): If both
> 	BFDs have identical build-ids, then they match.
> 	(_bfd_elf_core_find_build_id): New function.
> 	* elfxx-target.h (elf_backend_core_find_build_id): Define.
> 	(elfNN_bed): Add elf_backend_core_find_build_id.
> ---
>  bfd/elf-bfd.h      |   8 ++++
>  bfd/elf.c          |  23 +++++++---
>  bfd/elfcore.h      | 106 +++++++++++++++++++++++++++++++++++++++++++++
>  bfd/elfxx-target.h |   4 ++
>  4 files changed, 136 insertions(+), 5 deletions(-)
> 
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index 0a83c17332..2d214be87f 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1377,6 +1377,8 @@ struct elf_backend_data
>       int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
>  				bfd_size_type len));
>  
> +  bfd_boolean (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
> +
>    /* This function is used by `_bfd_elf_get_synthetic_symtab';
>       see elf.c.  */
>    bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
> @@ -2398,6 +2400,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf32_core_file_pid
>    (bfd *);
> +extern bfd_boolean _bfd_elf32_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf32_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2444,6 +2448,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf64_core_file_pid
>    (bfd *);
> +extern bfd_boolean _bfd_elf64_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf64_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2768,6 +2774,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
>  extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
> +extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> +				   size_t align) ;
>  
>  extern bfd_boolean _bfd_elf_parse_gnu_properties
>    (bfd *, Elf_Internal_Note *);
> diff --git a/bfd/elf.c b/bfd/elf.c
> index 664eae5c66..2ae3ec53c8 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
>  static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
>  static bfd_boolean prep_headers (bfd *);
>  static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
> -static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> -				   size_t align) ;
>  static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
>  				    file_ptr offset, size_t align);
>  
> @@ -3060,6 +3058,16 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
>    return TRUE;
>  }
>  
> +static bfd_boolean
> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
> +{
> +  /* The return value is ignored.  Build-ids are considered optional.  */
> +  if (templ->xvec->flavour == bfd_target_elf_flavour)
> +    return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
> +      (templ, offset);
> +  return FALSE;
> +}
> +
>  bfd_boolean
>  bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>  {
> @@ -3071,7 +3079,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>  
>      case PT_LOAD:
> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
> +	return FALSE;
> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
> +      return TRUE;
>  
>      case PT_DYNAMIC:
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
> @@ -11790,7 +11802,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>  	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
>  	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
>  	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
> -	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
> +	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
> +	      GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
>  	    };
>  #undef GROKER_ELEMENT
>  	    int i;
> @@ -11830,7 +11843,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>    return TRUE;
>  }
>  
> -static bfd_boolean
> +bfd_boolean
>  elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
>  		size_t align)
>  {
> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
> index 3550eaac27..751e831914 100644
> --- a/bfd/elfcore.h
> +++ b/bfd/elfcore.h
> @@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
>        return FALSE;
>      }
>  
> +  /* If both BFDs have identical build-ids, then they match.  */
> +  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
> +      && core_bfd->build_id->size == exec_bfd->build_id->size
> +      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
> +		 core_bfd->build_id->size) == 0)
> +    return TRUE;
> +
>    /* See if the name in the corefile matches the executable name.  */
>    corename = elf_tdata (core_bfd)->core->program;
>    if (corename != NULL)
> @@ -313,3 +320,102 @@ wrong:
>  fail:
>    return NULL;
>  }
> +
> +/* Attempt to find a build-id in a core file from the core file BFD.
> +   OFFSET is the file offset to a PT_LOAD segment that may contain
> +   the build-id note.  Returns TRUE upon success, FALSE otherwise.  */
> +
> +bfd_boolean
> +NAME(_bfd_elf,core_find_build_id)
> +  (bfd *abfd,
> +   bfd_vma offset)
> +{
> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form.   */
> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form.   */
> +  Elf_Internal_Phdr *i_phdr;
> +  unsigned int i;
> +
> +  /* Seek to the position of the segment at OFFSET.  */
> +  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
> +    goto fail;
> +
> +  /* Read in the ELF header in external format.  */
> +  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
> +    {
> +      if (bfd_get_error () != bfd_error_system_call)
> +	goto wrong;
> +      else
> +	goto fail;
> +    }
> +
> +  /* Now check to see if we have a valid ELF file, and one that BFD can
> +     make use of.  The magic number must match, the address size ('class')
> +     and byte-swapping must match our XVEC entry, and it must have a
> +     section header table (FIXME: See comments re sections at top of this
> +     file).  */
> +
> +  if (! elf_file_p (&x_ehdr)
> +      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
> +      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
> +    goto wrong;
> +
> +  /* Check that file's byte order matches xvec's */
> +  switch (x_ehdr.e_ident[EI_DATA])
> +    {
> +    case ELFDATA2MSB:		/* Big-endian */
> +      if (! bfd_header_big_endian (abfd))
> +	goto wrong;
> +      break;
> +    case ELFDATA2LSB:		/* Little-endian */
> +      if (! bfd_header_little_endian (abfd))
> +	goto wrong;
> +      break;
> +    case ELFDATANONE:		/* No data encoding specified */
> +    default:			/* Unknown data encoding specified */
> +      goto wrong;
> +    }
> +
> +  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
> +#if DEBUG & 1
> +  elf_debug_file (&i_ehdr);
> +#endif
> +
> +  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
> +    goto fail;
> +
> +  /* Read in program headers.  */
> +  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
> +					      sizeof (*i_phdr));
> +  if (i_phdr == NULL)
> +    goto fail;
> +
> +  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
> +    goto fail;
> +
> +  /* Read in program headers and parse notes.  */
> +  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
> +    {
> +      Elf_External_Phdr x_phdr;
> +
> +      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
> +	goto fail;
> +      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
> +
> +      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
> +	{
> +	  elf_read_notes (abfd, offset + i_phdr->p_offset,
> +			  i_phdr->p_filesz, i_phdr->p_align);
> +	  if (abfd->build_id != NULL)
> +	    return TRUE;
> +	}
> +    }
> +
> +  /* Having gotten this far, we have a valid ELF section, but no
> +     build-id was found.  */
> +  goto fail;
> +
> +wrong:
> +  bfd_set_error (bfd_error_wrong_format);
> +fail:
> +  return FALSE;
> +}
> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
> index 78a1f6314d..29ad2e8481 100644
> --- a/bfd/elfxx-target.h
> +++ b/bfd/elfxx-target.h
> @@ -521,6 +521,9 @@
>  #ifndef elf_backend_bfd_from_remote_memory
>  #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
>  #endif
> +#ifndef elf_backend_core_find_build_id
> +#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
> +#endif
>  #ifndef elf_backend_got_header_size
>  #define elf_backend_got_header_size	0
>  #endif
> @@ -860,6 +863,7 @@ static struct elf_backend_data elfNN_bed =
>    elf_backend_mips_rtype_to_howto,
>    elf_backend_ecoff_debug_swap,
>    elf_backend_bfd_from_remote_memory,
> +  elf_backend_core_find_build_id,
>    elf_backend_plt_sym_val,
>    elf_backend_common_definition,
>    elf_backend_common_section_index,
>