Re: [PATCH] readelf: Support --dynamic with --use-dynamic

[Mark Wielaard <mark@klomp.org>]

Hi,

On Thu, May 05, 2022 at 09:01:24PM +0800, Di Chen via Elfutils-devel wrote:
> From 3ac23c2584d76114deab0c0af6f4af99068dc7f4 Mon Sep 17 00:00:00 2001
> From: Di Chen <dichen@redhat.com>
> Date: Thu, 28 Apr 2022 19:55:33 +0800
> Subject: [PATCH] readelf: Support --dynamic with --use-dynamic
> 
> Currently, eu-readelf is using section headers to dump the dynamic
> segment information (print_dynamic -> handle_dynamic).
> 
> This patch adds new options to eu-readelf (-D, --use-dynamic)
> for (-d, --dynamic).
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28873

This looks pretty good. But there are lots of white-space issues which
make it hard to apply the patch. Could you sent it again preferrably
using git send-email ?

> Signed-off-by: Di Chen <dichen@redhat.com>
> ---
>  src/readelf.c           | 168 +++++++++++++++++++++++++++++++++++++---
>  tests/Makefile.am       |   3 +-
>  tests/run-readelf-Dd.sh |  65 ++++++++++++++++
>  3 files changed, 223 insertions(+), 13 deletions(-)
>  create mode 100755 tests/run-readelf-Dd.sh
> 
> diff --git a/src/readelf.c b/src/readelf.c
> index 4b6aab2b..e578456b 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -137,6 +137,8 @@ static const struct argp_option options[] =
>    { "string-dump", 'p', NULL, OPTION_ALIAS | OPTION_HIDDEN, NULL, 0 },
>    { "archive-index", 'c', NULL, 0,
>      N_("Display the symbol index of an archive"), 0 },
> +  { "use-dynamic", 'D', NULL, 0,
> +    N_("Use the dynamic section info when displaying symbols"), 0 },

This doesn't handle symbols yet. Should it simply say:
"Use the dynamic segment when possible for displaying info"

>    { NULL, 0, NULL, 0, N_("Output control:"), 0 },
>    { "numeric-addresses", 'N', NULL, 0,
> @@ -195,6 +197,9 @@ static bool print_symbol_table;
>  /* True if (only) the dynsym table should be printed.  */
>  static bool print_dynsym_table;
> 
> +/* True if reconstruct dynamic symbol table from the PT_DYNAMIC segment.
>  */
> +static bool use_dynamic_segment;
> +
>  /* A specific section name, or NULL to print all symbol tables.  */
>  static char *symbol_table_section;
> 
> @@ -268,6 +273,19 @@ static enum section_e
>   | section_macro | section_addr | section_types)
>  } print_debug_sections, implicit_debug_sections;
> 
> +enum dyn_idx
> +{
> +  i_strsz,
> +  i_verneed,
> +  i_verdef,
> +  i_versym,
> +  i_symtab,
> +  i_strtab,
> +  i_hash,
> +  i_gnu_hash,
> +  i_max
> +};

Maybe move this just before it is used in declarations of the
get_dyn... functions?

>  /* Select hex dumping of sections.  */
>  static struct section_argument *dump_data_sections;
>  static struct section_argument **dump_data_sections_tail =
> &dump_data_sections;
> @@ -318,6 +336,11 @@ static void dump_strings (Ebl *ebl);
>  static void print_strings (Ebl *ebl);
>  static void dump_archive_index (Elf *, const char *);
> 
> +/* Declarations of local functions for use-dynamic.  */
> +static Elf_Data * get_dynscn_strtab(Elf *elf, GElf_Phdr *phdr);
> +static void get_dynscn_addrs(Elf *elf, GElf_Phdr *phdr, GElf_Addr
> addrs[i_max]);
> +static void find_offsets(Elf *elf, GElf_Addr main_bias, size_t n,
> +                GElf_Addr addrs[n], GElf_Off offs[n]);

I mean, just before these.

>  /* Looked up once with gettext in main.  */
>  static char *yes_str;
> @@ -429,6 +452,9 @@ parse_opt (int key, char *arg,
>        print_dynamic_table = true;
>        any_control_option = true;
>        break;
> +    case 'D':
> +      use_dynamic_segment = true;
> +      break;
>      case 'e':
>        print_debug_sections |= section_exception;
>        any_control_option = true;
> @@ -1791,7 +1817,7 @@ get_dyn_ents (Elf_Data * dyn_data)

OK, setting flag.

> 
>  static void
> -handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
> +handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr, GElf_Phdr *phdr)
>  {
>    int class = gelf_getclass (ebl->elf);
>    GElf_Shdr glink_mem;
> @@ -1802,13 +1828,20 @@ handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr
> *shdr)
>    size_t dyn_ents;
> 
>    /* Get the data of the section.  */
> -  data = elf_getdata (scn, NULL);
> +  if (use_dynamic_segment)
> +    data = elf_getdata_rawchunk(
> +  ebl->elf, phdr->p_offset, phdr->p_filesz, ELF_T_DYN);
> +  else
> +    data = elf_getdata (scn, NULL);
> +
>    if (data == NULL)
>      return;
> 
>    /* Get the dynamic section entry number */
>    dyn_ents = get_dyn_ents (data);
> 
> +  if (!use_dynamic_segment)
> +{
>    /* Get the section header string table index.  */
>    if (unlikely (elf_getshdrstrndx (ebl->elf, &shstrndx) < 0))
>      error_exit (0, _("cannot get section header string table index"));
> @@ -1828,8 +1861,25 @@ handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr
> *shdr)
>    shdr->sh_offset,
>    (int) shdr->sh_link,
>    elf_strptr (ebl->elf, shstrndx, glink->sh_name));
> +} else {
> +  printf (ngettext ("\
> +\nDynamic segment contains %lu entry:\n Addr: %#0*" PRIx64 "  Offset:
> %#08" PRIx64 "\n",
> +    "\
> +\nDynamic segment contains %lu entries:\n Addr: %#0*" PRIx64 "  Offset:
> %#08" PRIx64 "\n",
> +    dyn_ents),
> +  (unsigned long int) dyn_ents,
> +  class == ELFCLASS32 ? 10 : 18, phdr->p_paddr,
> +  phdr->p_offset);
> +}
> +
>    fputs_unlocked (_("  Type              Value\n"), stdout);
> 
> +  /* if --use-dynamic option is enabled,
> +     use the string table to get the related library info.  */
> +  Elf_Data *strtab_data = NULL;
> +  if (use_dynamic_segment)
> +    strtab_data = get_dynscn_strtab(ebl->elf, phdr);
> +
>    for (cnt = 0; cnt < dyn_ents; ++cnt)
>      {
>        GElf_Dyn dynmem;
> @@ -1841,6 +1891,12 @@ handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr
> *shdr)
>        printf ("  %-17s ",
>        ebl_dynamic_tag_name (ebl, dyn->d_tag, buf, sizeof (buf)));
> 
> +      char *lib_name = NULL;
> +      if (use_dynamic_segment)
> +    lib_name = ((char *)strtab_data->d_buf) + dyn->d_un.d_ptr;

I think strtab_data can be NULL here (if get_dynscn_strtab failed) and
dyn->d_un.d_ptr could be beyond the end of data (if the ELF file is
bogus).

> +      else
> +    lib_name = elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val);
> +
>        switch (dyn->d_tag)
>   {
>   case DT_NULL:
> @@ -1852,23 +1908,19 @@ handle_dynamic (Ebl *ebl, Elf_Scn *scn, GElf_Shdr
> *shdr)
>    break;
> 
>   case DT_NEEDED:
> -  printf (_("Shared library: [%s]\n"),
> -  elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val));
> +  printf (_("Shared library: [%s]\n"), lib_name);
>    break;
> 
>   case DT_SONAME:
> -  printf (_("Library soname: [%s]\n"),
> -  elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val));
> +  printf (_("Library soname: [%s]\n"),lib_name);
>    break;
> 
>   case DT_RPATH:
> -  printf (_("Library rpath: [%s]\n"),
> -  elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val));
> +  printf (_("Library rpath: [%s]\n"),lib_name);
>    break;
> 
>   case DT_RUNPATH:
> -  printf (_("Library runpath: [%s]\n"),
> -  elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val));
> +  printf (_("Library runpath: [%s]\n"), lib_name);
>    break;

OK.

>   case DT_PLTRELSZ:
> @@ -1942,8 +1994,8 @@ print_dynamic (Ebl *ebl)
>    Elf_Scn *scn = gelf_offscn (ebl->elf, phdr->p_offset);
>    GElf_Shdr shdr_mem;
>    GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
> -  if (shdr != NULL && shdr->sh_type == SHT_DYNAMIC)
> -    handle_dynamic (ebl, scn, shdr);
> +  if (use_dynamic_segment || (shdr != NULL && shdr->sh_type ==
> SHT_DYNAMIC))
> +    handle_dynamic (ebl, scn, shdr, phdr);
>    break;

Should you check that phdr != NULL here?

>   }
>      }
> @@ -4801,6 +4853,98 @@ print_ops (Dwfl_Module *dwflmod, Dwarf *dbg, int
> indent, int indentrest,
>  }
> 
> 
> +static void
> +find_offsets(Elf *elf, GElf_Addr main_bias, size_t n,
> +                  GElf_Addr addrs[n], GElf_Off offs[n])
> +{
> +  size_t unsolved = n;
> +  for (size_t i = 0; i < phnum; ++i) {
> +    GElf_Phdr phdr_mem;
> +    GElf_Phdr *phdr = gelf_getphdr(elf, i, &phdr_mem);
> +    if (phdr != NULL && phdr->p_type == PT_LOAD && phdr->p_memsz > 0)
> +      for (size_t j = 0; j < n; ++j)
> +        if (offs[j] == 0 && addrs[j] >= phdr->p_vaddr + main_bias &&
> +            addrs[j] - (phdr->p_vaddr + main_bias) < phdr->p_filesz) {
> +          offs[j] = addrs[j] - (phdr->p_vaddr + main_bias) +
> phdr->p_offset;
> +          if (--unsolved == 0)
> +            break;
> +        }
> +  }
> +}

OK, so this turns the addresses into file offsets using the phdrs.  It
is slightly tricky, so I wouldn't mind a comment explaining what is
going on.

> +/* The dynamic segment (type PT_DYNAMIC), contains the .dynamic section.
> +   And .dynamic section contains an array of the dynamic structures.
> +   We use the array to get:
> +    DT_STRTAB: the address of the string table.
> + DT_SYMTAB: the address of the symbol table.
> + DT_STRSZ: the size, in bytes, of the string table.
> + , and etc.  */
> +static void
> +get_dynscn_addrs(Elf *elf, GElf_Phdr *phdr, GElf_Addr addrs[i_max])
> +{
> +  Elf_Data *data = elf_getdata_rawchunk(
> +    elf, phdr->p_offset, phdr->p_filesz, ELF_T_DYN);
> +
> +  int dyn_idx = 0;
> +  for (;; ++dyn_idx) {
> +    GElf_Dyn dyn_mem;
> +    GElf_Dyn *dyn = gelf_getdyn(data, dyn_idx, &dyn_mem);
> +    /* DT_NULL Marks end of dynamic section.  */
> +    if (dyn->d_tag == DT_NULL)
> +      break;
> +
> +    switch (dyn->d_tag) {
> +    case DT_SYMTAB:
> +      addrs[i_symtab] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_HASH:
> +      addrs[i_hash] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_GNU_HASH:
> +      addrs[i_gnu_hash] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_STRTAB:
> +      addrs[i_strtab] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_VERSYM:
> +      addrs[i_versym] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_VERDEF:
> +      addrs[i_verdef] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_VERNEED:
> +      addrs[i_verneed] = dyn->d_un.d_ptr;
> +      break;
> +
> +    case DT_STRSZ:
> +      addrs[i_strsz] = dyn->d_un.d_val;
> +    }
> +  }
> +  return;
> +}

Note that the return here is redundant and could be left off.

> +/* Use dynamic segment to get data for the string table section.  */
> +static Elf_Data *
> +get_dynscn_strtab(Elf *elf, GElf_Phdr *phdr)
> +{
> +  Elf_Data *strtab_data;
> +  GElf_Addr addrs[i_max] = {0,};
> +  GElf_Off offs[i_max] = {0,};
> +  get_dynscn_addrs(elf, phdr, addrs);
> +  find_offsets(elf, 0, i_max, addrs, offs);
> +  strtab_data = elf_getdata_rawchunk(
> +          elf, offs[i_strtab], addrs[i_strsz], ELF_T_BYTE);
> +  return strtab_data;
> +}

OK.

> +
>  struct listptr
>  {
>    Dwarf_Off offset:(64 - 3);
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 84c3950a..b239a461 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -197,7 +197,8 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh
> newfile test-nlist \
>   msg_tst system-elf-libelf-test \
>   $(asm_TESTS) run-disasm-bpf.sh run-low_high_pc-dw-form-indirect.sh \
>   run-nvidia-extended-linemap-libdw.sh
> run-nvidia-extended-linemap-readelf.sh \
> - run-readelf-dw-form-indirect.sh run-strip-largealign.sh
> + run-readelf-dw-form-indirect.sh run-strip-largealign.sh \
> + run-readelf-Dd.sh

Don't forget to add run-readelf-Dd.sh to EXTRA_DISTS.
(make distcheck can be used to check for that)

>  if !BIARCH
>  export ELFUTILS_DISABLE_BIARCH = 1
> diff --git a/tests/run-readelf-Dd.sh b/tests/run-readelf-Dd.sh
> new file mode 100755
> index 00000000..6420a0fe
> --- /dev/null
> +++ b/tests/run-readelf-Dd.sh
> @@ -0,0 +1,65 @@
> +#! /bin/sh
> +# Copyright (C) 2022 Red Hat, Inc.
> +# This file is part of elfutils.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# elfutils is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. $srcdir/test-subr.sh
> +
> +# #include <stdio.h>
> +#
> +# __thread int i;
> +#
> +# void print_i ()
> +# {
> +#   printf("%d\n", i);
> +# }
> +#
> +# gcc -fPIC -shared -o testlib_dynseg.so testlib_dynseg.c
> +# With ld --version
> +# GNU gold (GNU Binutils 2.22.52.20120402) 1.11
> +
> +testfiles testlib_dynseg.so

Maybe add a comment saying the same testfile is used in run-readelf-d.sh

> +testrun_compare ${abs_top_builddir}/src/readelf -Dd testlib_dynseg.so
> <<\EOF
> +
> +Dynamic segment contains 23 entries:
> + Addr: 0x00000000000017e0  Offset: 0x0007e0
> +  Type              Value
> +  PLTGOT            0x00000000000019c8
> +  PLTRELSZ          72 (bytes)
> +  JMPREL            0x0000000000000568
> +  PLTREL            RELA
> +  RELA              0x00000000000004d8
> +  RELASZ            144 (bytes)
> +  RELAENT           24 (bytes)
> +  RELACOUNT         1
> +  SYMTAB            0x0000000000000228
> +  SYMENT            24 (bytes)
> +  STRTAB            0x0000000000000360
> +  STRSZ             190 (bytes)
> +  GNU_HASH          0x0000000000000420
> +  NEEDED            Shared library: [libc.so.6]
> +  NEEDED            Shared library: [ld-linux-x86-64.so.2]
> +  INIT              0x00000000000005b0
> +  FINI              0x0000000000000748
> +  VERSYM            0x0000000000000460
> +  VERDEF            0x000000000000047c
> +  VERDEFNUM         1
> +  VERNEED           0x0000000000000498
> +  VERNEEDNUM        2
> +  NULL
> +EOF
> +
> +exit 0
> -- 
> 2.35.1