readelf: Support --dynamic with --use-dynamic

  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

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

+exit 0
  

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
  

Thanks Mark,

All the request changes are fixed, ready for review again.

1. help message updated: "Use the dynamic segment when possible for
displaying info"
2. move enum dyn_idx to a proper place
3. add strtab_data's NULL check in function: handle_dynamic()
4. add phdr's NULL check in function: print_dynamic()
5. add comments for function: find_offsets()
6. remove redundant return-statement in function: get_dynscn_addrs()
7. add run-readelf-Dd.sh to EXTRA_DISTS
8. check strsz in (dyn->d_un.d_ptr < strtab_data->d_size) in function:
handle_dynamic()


On Fri, May 20, 2022 at 8:41 AM Mark Wielaard <mark@klomp.org> wrote:

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

Hi,

On Tue, May 24, 2022 at 11:53:11PM +0800, Di Chen via Elfutils-devel wrote:
> All the request changes are fixed, ready for review again.
> 
> 1. help message updated: "Use the dynamic segment when possible for
> displaying info"
> 2. move enum dyn_idx to a proper place
> 3. add strtab_data's NULL check in function: handle_dynamic()
> 4. add phdr's NULL check in function: print_dynamic()
> 5. add comments for function: find_offsets()
> 6. remove redundant return-statement in function: get_dynscn_addrs()
> 7. add run-readelf-Dd.sh to EXTRA_DISTS
> 8. check strsz in (dyn->d_un.d_ptr < strtab_data->d_size) in function:
> handle_dynamic()

Sorry the re-review took so long. This looks great. I did add a NEWS
entry and wrote a Changelog entry while re-reviewing. And a few small
whitespace fixups.

The only code change I made was:

-      char *lib_name = NULL;
-
-      if (!use_dynamic_segment)
-    lib_name = elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val);
-      else if (use_dynamic_segment)
-    lib_name = ((char *)strtab_data->d_buf) + dyn->d_un.d_ptr;
-      else
-    break;
+      char *name = NULL;
+      if (dyn->d_tag == DT_NEEDED
+         || dyn->d_tag == DT_SONAME
+         || dyn->d_tag == DT_RPATH
+         || dyn->d_tag == DT_RUNPATH)
+       {
+         if (! use_dynamic_segment)
+           name = elf_strptr (ebl->elf, shdr->sh_link, dyn->d_un.d_val);
+         else if (dyn->d_un.d_ptr < strtab_data->d_size
+                  && memrchr (strtab_data->d_buf + strtab_data->d_size - 1, '\0',
+                              strtab_data->d_size - 1 - dyn->d_un.d_ptr) != NULL)
+           name = ((char *) strtab_data->d_buf) + dyn->d_un.d_ptr;
+       }

That does the check whether dyn->d_un.d_ptr is valid early, so it
doesn't need to be checked in each case statement. Also it adds an
extra memrchr check to make sure the string is zero terminated.

Pushed with those changes.

Thanks,

Mark