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 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 > > 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 > > --- > > 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 . > > + > > +. $srcdir/test-subr.sh > > + > > +# #include > > +# > > +# __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 > >