* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header [not found] ` <20230606175846.399377-4-hjl.tools@gmail.com> @ 2023-07-01 2:12 ` Simon Marchi 2023-07-07 15:26 ` H.J. Lu 0 siblings, 1 reply; 6+ messages in thread From: Simon Marchi @ 2023-07-01 2:12 UTC (permalink / raw) To: H.J. Lu, binutils; +Cc: Alan Modra, Florian Weimer, Kaylee Blake, gdb-patches On 6/6/23 13:58, H.J. Lu wrote: > When there is no section header in an executable or shared library, we > reconstruct dynamic symbol table from the PT_DYNAMIC segment, which > contains DT_HASH/DT_GNU_HASH/DT_MIPS_XHASH, DT_STRTAB, DT_SYMTAB, > DT_STRSZ, and DT_SYMENT entries, to improve nm and objdump. For DT_HASH, > the number of dynamic symbol table entries equals the number of chains. > For DT_GNU_HASH/DT_MIPS_XHASH, only defined symbols with non-STB_LOCAL > indings are in hash table. Since DT_GNU_HASH/DT_MIPS_XHASH place all > symbols with STB_LOCAL binding before symbols with other bindings and > all undefined symbols defined ones in dynamic symbol table, the highest > symbol index in DT_GNU_HASH/DT_MIPS_XHASH is the highest dynamic symbol > table index. We can also get symbol version from DT_VERSYM, DT_VERDEF > and DT_VERNEED entries. > > dt_symtab, dt_versym, dt_verdef, dt_verneed, dt_symtab_count, > dt_verdef_count, dt_verneed_count and dt_strtab are added to > elf_obj_tdata to store dynamic symbol table information. Hi, This broke a GDB test: gdb/ $ make check TESTS="gdb.base/eu-strip-infcall.exp" FAIL: gdb.base/eu-strip-infcall.exp: gdb_breakpoint: set breakpoint at main Looking at gdb/testsuite/gdb.log, before: (gdb) file /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall... Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug... and after: (gdb) file /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall... warning: `/home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug': can't read symbols: file format not recognized. (No debugging symbols found in /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall) Can you please check if it's a problem with your patch, or if it's really a malformed file? Thanks, Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header 2023-07-01 2:12 ` [PATCH v4 3/7] bfd: Improve nm and objdump without section header Simon Marchi @ 2023-07-07 15:26 ` H.J. Lu 2023-07-10 3:30 ` Simon Marchi 0 siblings, 1 reply; 6+ messages in thread From: H.J. Lu @ 2023-07-07 15:26 UTC (permalink / raw) To: Simon Marchi Cc: binutils, Alan Modra, Florian Weimer, Kaylee Blake, gdb-patches On Fri, Jun 30, 2023 at 7:12 PM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > On 6/6/23 13:58, H.J. Lu wrote: > > When there is no section header in an executable or shared library, we > > reconstruct dynamic symbol table from the PT_DYNAMIC segment, which > > contains DT_HASH/DT_GNU_HASH/DT_MIPS_XHASH, DT_STRTAB, DT_SYMTAB, > > DT_STRSZ, and DT_SYMENT entries, to improve nm and objdump. For DT_HASH, > > the number of dynamic symbol table entries equals the number of chains. > > For DT_GNU_HASH/DT_MIPS_XHASH, only defined symbols with non-STB_LOCAL > > indings are in hash table. Since DT_GNU_HASH/DT_MIPS_XHASH place all > > symbols with STB_LOCAL binding before symbols with other bindings and > > all undefined symbols defined ones in dynamic symbol table, the highest > > symbol index in DT_GNU_HASH/DT_MIPS_XHASH is the highest dynamic symbol > > table index. We can also get symbol version from DT_VERSYM, DT_VERDEF > > and DT_VERNEED entries. > > > > dt_symtab, dt_versym, dt_verdef, dt_verneed, dt_symtab_count, > > dt_verdef_count, dt_verneed_count and dt_strtab are added to > > elf_obj_tdata to store dynamic symbol table information. > > Hi, > > This broke a GDB test: > > gdb/ $ make check TESTS="gdb.base/eu-strip-infcall.exp" > FAIL: gdb.base/eu-strip-infcall.exp: gdb_breakpoint: set breakpoint at main > > Looking at gdb/testsuite/gdb.log, before: > > (gdb) file /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall > > Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall... > > Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug... > > > and after: > > (gdb) file /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall > > Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall... > > warning: `/home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug': can't read symbols: file format not recognized. > > (No debugging symbols found in /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall) > > > Can you please check if it's a problem with your patch, or if it's really a malformed file? > > Thanks, > > Simon It works for me: $ make check TESTS="gdb.base/eu-strip-infcall.exp" .... === gdb Summary === # of expected passes 1 My change only impacts files without section header. eu-strip-infcall.exp does "eu-strip -f ${binfile}.debug $binfile", which doesn't remove section header. -- H.J. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header 2023-07-07 15:26 ` H.J. Lu @ 2023-07-10 3:30 ` Simon Marchi 2023-07-13 5:02 ` Alan Modra 0 siblings, 1 reply; 6+ messages in thread From: Simon Marchi @ 2023-07-10 3:30 UTC (permalink / raw) To: H.J. Lu; +Cc: binutils, Alan Modra, Florian Weimer, Kaylee Blake, gdb-patches > It works for me: > > $ make check TESTS="gdb.base/eu-strip-infcall.exp" > .... > === gdb Summary === > > # of expected passes 1 > > My change only impacts files without section header. eu-strip-infcall.exp does > "eu-strip -f ${binfile}.debug $binfile", which doesn't remove section header. > I can reliably reproduce the problem on two separate machine, one Ubuntu 22.04 and one failrly up to date Arch Linux. elfutils version 0.186 and 0.189, respectively. It goes wrong when GDB does a bfd_check_format call on testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug. Before you commit it works, and after your commit it returns false. It happens in this new statement added to elf_object_p, added by the commit: if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize) goto got_no_match; (top-gdb) p i_phdr->p_offset $1 = 8192 (top-gdb) p i_phdr->p_filesz $2 = 196 (top-gdb) p filesize $3 = 5104 (top-gdb) p i $4 = 4 It would be this program header causing the condition to fail: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align ... LOAD 0x002000 0x0000000000002000 0x0000000000002000 0x0000c4 0x0000c4 R 0x1000 So, the program header of the .debug file describes the segments of the main binary, not sure if that's expected. Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header 2023-07-10 3:30 ` Simon Marchi @ 2023-07-13 5:02 ` Alan Modra 2023-07-13 5:34 ` Fangrui Song 2023-07-13 21:58 ` Mark Wielaard 0 siblings, 2 replies; 6+ messages in thread From: Alan Modra @ 2023-07-13 5:02 UTC (permalink / raw) To: Simon Marchi; +Cc: H.J. Lu, binutils, Florian Weimer, Kaylee Blake, gdb-patches On Sun, Jul 09, 2023 at 11:30:01PM -0400, Simon Marchi wrote: > > > It works for me: > > > > $ make check TESTS="gdb.base/eu-strip-infcall.exp" > > .... > > === gdb Summary === > > > > # of expected passes 1 > > > > My change only impacts files without section header. eu-strip-infcall.exp does > > "eu-strip -f ${binfile}.debug $binfile", which doesn't remove section header. > > > > I can reliably reproduce the problem on two separate machine, one Ubuntu > 22.04 and one failrly up to date Arch Linux. elfutils version 0.186 and > 0.189, respectively. > > It goes wrong when GDB does a bfd_check_format call on > testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug. > Before you commit it works, and after your commit it returns false. It > happens in this new statement added to elf_object_p, added by the commit: > > if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize) > goto got_no_match; > > (top-gdb) p i_phdr->p_offset > $1 = 8192 > (top-gdb) p i_phdr->p_filesz > $2 = 196 > (top-gdb) p filesize > $3 = 5104 > (top-gdb) p i > $4 = 4 > > It would be this program header causing the condition to fail: > > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > ... > LOAD 0x002000 0x0000000000002000 0x0000000000002000 0x0000c4 0x0000c4 R 0x1000 > > So, the program header of the .debug file describes the segments of the > main binary, not sure if that's expected. No, that's not expected. Program headers in a .debug file ought to describe the contents of the debug file. You'll typically see many with p_filesz zero. eu-strip appears to be broken in this respect. There is another problem with the code added to elf_object_p: _bfd_elf_get_dynamic_symbols is told that it can access up to e_phnum program headers, but they very likely haven't all been swapped in. I'm going to apply the following patch. ---- elf_object_p load of dynamic symbols This fixes an uninitialised memory access on a fuzzed file: 0 0xf22e9b in offset_from_vma /src/binutils-gdb/bfd/elf.c:1899:2 1 0xf1e90f in _bfd_elf_get_dynamic_symbols /src/binutils-gdb/bfd/elf.c:2099:13 2 0x10e6a54 in bfd_elf32_object_p /src/binutils-gdb/bfd/elfcode.h:851:9 Hopefully it will also stop any attempt to load dynamic symbols from eu-strip debug files. * elfcode.h (elf_object_p): Do not attempt to load dynamic symbols for a file with no section headers until all the program headers are swapped in. Do not fail on eu-strip debug files. diff --git a/bfd/elfcode.h b/bfd/elfcode.h index aae66bcebf8..b2277921680 100644 --- a/bfd/elfcode.h +++ b/bfd/elfcode.h @@ -819,6 +819,7 @@ elf_object_p (bfd *abfd) goto got_no_match; if (bfd_seek (abfd, (file_ptr) i_ehdrp->e_phoff, SEEK_SET) != 0) goto got_no_match; + bool eu_strip_broken_phdrs = false; i_phdr = elf_tdata (abfd)->phdr; for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++) { @@ -839,21 +840,31 @@ elf_object_p (bfd *abfd) abfd->read_only = 1; } } - if (i_phdr->p_filesz != 0) - { - if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize) - goto got_no_match; - /* Try to reconstruct dynamic symbol table from PT_DYNAMIC - segment if there is no section header. */ - if (i_phdr->p_type == PT_DYNAMIC - && i_ehdrp->e_shstrndx == 0 - && i_ehdrp->e_shoff == 0 - && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr, - elf_tdata (abfd)->phdr, - i_ehdrp->e_phnum, - filesize)) - goto got_no_match; - } + /* Detect eu-strip -f debug files, which have program + headers that describe the original file. */ + if (i_phdr->p_filesz != 0 + && (i_phdr->p_filesz > filesize + || i_phdr->p_offset > filesize - i_phdr->p_filesz)) + eu_strip_broken_phdrs = true; + } + if (!eu_strip_broken_phdrs + && i_ehdrp->e_shoff == 0 + && i_ehdrp->e_shstrndx == 0) + { + /* Try to reconstruct dynamic symbol table from PT_DYNAMIC + segment if there is no section header. */ + i_phdr = elf_tdata (abfd)->phdr; + for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++) + if (i_phdr->p_type == PT_DYNAMIC) + { + if (i_phdr->p_filesz != 0 + && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr, + elf_tdata (abfd)->phdr, + i_ehdrp->e_phnum, + filesize)) + goto got_no_match; + break; + } } } -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header 2023-07-13 5:02 ` Alan Modra @ 2023-07-13 5:34 ` Fangrui Song 2023-07-13 21:58 ` Mark Wielaard 1 sibling, 0 replies; 6+ messages in thread From: Fangrui Song @ 2023-07-13 5:34 UTC (permalink / raw) To: Alan Modra, Simon Marchi Cc: H.J. Lu, binutils, Florian Weimer, Kaylee Blake, gdb-patches On Wed, Jul 12, 2023 at 10:02 PM Alan Modra via Binutils <binutils@sourceware.org> wrote: > > On Sun, Jul 09, 2023 at 11:30:01PM -0400, Simon Marchi wrote: > > > > > It works for me: > > > > > > $ make check TESTS="gdb.base/eu-strip-infcall.exp" > > > .... > > > === gdb Summary === > > > > > > # of expected passes 1 > > > > > > My change only impacts files without section header. eu-strip-infcall.exp does > > > "eu-strip -f ${binfile}.debug $binfile", which doesn't remove section header. > > > > > > > I can reliably reproduce the problem on two separate machine, one Ubuntu > > 22.04 and one failrly up to date Arch Linux. elfutils version 0.186 and > > 0.189, respectively. > > > > It goes wrong when GDB does a bfd_check_format call on > > testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug. > > Before you commit it works, and after your commit it returns false. It > > happens in this new statement added to elf_object_p, added by the commit: > > > > if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize) > > goto got_no_match; > > > > (top-gdb) p i_phdr->p_offset > > $1 = 8192 > > (top-gdb) p i_phdr->p_filesz > > $2 = 196 > > (top-gdb) p filesize > > $3 = 5104 > > (top-gdb) p i > > $4 = 4 > > > > It would be this program header causing the condition to fail: > > > > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > > ... > > LOAD 0x002000 0x0000000000002000 0x0000000000002000 0x0000c4 0x0000c4 R 0x1000 > > > > So, the program header of the .debug file describes the segments of the > > main binary, not sure if that's expected. > > No, that's not expected. Program headers in a .debug file ought to > describe the contents of the debug file. You'll typically see many > with p_filesz zero. eu-strip appears to be broken in this respect. Do you have a bug number for eu-strip ? It seems good to keep a reference of the bug so that we can track it. > There is another problem with the code added to elf_object_p: > _bfd_elf_get_dynamic_symbols is told that it can access up to e_phnum > program headers, but they very likely haven't all been swapped in. > > I'm going to apply the following patch. > ---- > > elf_object_p load of dynamic symbols > > This fixes an uninitialised memory access on a fuzzed file: > 0 0xf22e9b in offset_from_vma /src/binutils-gdb/bfd/elf.c:1899:2 > 1 0xf1e90f in _bfd_elf_get_dynamic_symbols /src/binutils-gdb/bfd/elf.c:2099:13 > 2 0x10e6a54 in bfd_elf32_object_p /src/binutils-gdb/bfd/elfcode.h:851:9 > > Hopefully it will also stop any attempt to load dynamic symbols from > eu-strip debug files. > > * elfcode.h (elf_object_p): Do not attempt to load dynamic > symbols for a file with no section headers until all the > program headers are swapped in. Do not fail on eu-strip debug > files. > > diff --git a/bfd/elfcode.h b/bfd/elfcode.h > index aae66bcebf8..b2277921680 100644 > --- a/bfd/elfcode.h > +++ b/bfd/elfcode.h > @@ -819,6 +819,7 @@ elf_object_p (bfd *abfd) > goto got_no_match; > if (bfd_seek (abfd, (file_ptr) i_ehdrp->e_phoff, SEEK_SET) != 0) > goto got_no_match; > + bool eu_strip_broken_phdrs = false; > i_phdr = elf_tdata (abfd)->phdr; > for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++) > { > @@ -839,21 +840,31 @@ elf_object_p (bfd *abfd) > abfd->read_only = 1; > } > } > - if (i_phdr->p_filesz != 0) > - { > - if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize) > - goto got_no_match; > - /* Try to reconstruct dynamic symbol table from PT_DYNAMIC > - segment if there is no section header. */ > - if (i_phdr->p_type == PT_DYNAMIC > - && i_ehdrp->e_shstrndx == 0 > - && i_ehdrp->e_shoff == 0 > - && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr, > - elf_tdata (abfd)->phdr, > - i_ehdrp->e_phnum, > - filesize)) > - goto got_no_match; > - } > + /* Detect eu-strip -f debug files, which have program > + headers that describe the original file. */ > + if (i_phdr->p_filesz != 0 > + && (i_phdr->p_filesz > filesize > + || i_phdr->p_offset > filesize - i_phdr->p_filesz)) > + eu_strip_broken_phdrs = true; > + } > + if (!eu_strip_broken_phdrs > + && i_ehdrp->e_shoff == 0 > + && i_ehdrp->e_shstrndx == 0) > + { > + /* Try to reconstruct dynamic symbol table from PT_DYNAMIC > + segment if there is no section header. */ > + i_phdr = elf_tdata (abfd)->phdr; > + for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++) > + if (i_phdr->p_type == PT_DYNAMIC) > + { > + if (i_phdr->p_filesz != 0 > + && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr, > + elf_tdata (abfd)->phdr, > + i_ehdrp->e_phnum, > + filesize)) > + goto got_no_match; > + break; > + } > } > } > > > -- > Alan Modra > Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header 2023-07-13 5:02 ` Alan Modra 2023-07-13 5:34 ` Fangrui Song @ 2023-07-13 21:58 ` Mark Wielaard 1 sibling, 0 replies; 6+ messages in thread From: Mark Wielaard @ 2023-07-13 21:58 UTC (permalink / raw) To: Alan Modra Cc: Simon Marchi, H.J. Lu, binutils, Florian Weimer, Kaylee Blake, gdb-patches, Ryan Goldberg, Matthias Klose, nickc Hi, On Thu, Jul 13, 2023 at 02:32:25PM +0930, Alan Modra via Binutils wrote: > > So, the program header of the .debug file describes the segments of the > > main binary, not sure if that's expected. > > No, that's not expected. Program headers in a .debug file ought to > describe the contents of the debug file. You'll typically see many > with p_filesz zero. eu-strip appears to be broken in this respect. It is by design that eu-strip -f copies over the program headers of the main file into the .debug file. It would be nice to tag .debug files as such, to prevent issues like this. There is a binutils bug about it: https://sourceware.org/bugzilla/show_bug.cgi?id=22136 > There is another problem with the code added to elf_object_p: > _bfd_elf_get_dynamic_symbols is told that it can access up to e_phnum > program headers, but they very likely haven't all been swapped in. > > I'm going to apply the following patch. > > ---- > > elf_object_p load of dynamic symbols > > This fixes an uninitialised memory access on a fuzzed file: > 0 0xf22e9b in offset_from_vma /src/binutils-gdb/bfd/elf.c:1899:2 > 1 0xf1e90f in _bfd_elf_get_dynamic_symbols /src/binutils-gdb/bfd/elf.c:2099:13 > 2 0x10e6a54 in bfd_elf32_object_p /src/binutils-gdb/bfd/elfcode.h:851:9 > > Hopefully it will also stop any attempt to load dynamic symbols from > eu-strip debug files. > > * elfcode.h (elf_object_p): Do not attempt to load dynamic > symbols for a file with no section headers until all the > program headers are swapped in. Do not fail on eu-strip debug > files. Thanks! This does resolves an elfutils/debuginfod issue Ryan and I were tracking down on debian-testing with using binutils objcopy extracting sections from a .debug file. Debian testing ships with binutils 2.40.90.20230705. If possible could this go into 2.41 (and in an update for Debian testing)? Thanks, Mark ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-13 21:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230606175846.399377-1-hjl.tools@gmail.com>
[not found] ` <20230606175846.399377-4-hjl.tools@gmail.com>
2023-07-01 2:12 ` [PATCH v4 3/7] bfd: Improve nm and objdump without section header Simon Marchi
2023-07-07 15:26 ` H.J. Lu
2023-07-10 3:30 ` Simon Marchi
2023-07-13 5:02 ` Alan Modra
2023-07-13 5:34 ` Fangrui Song
2023-07-13 21:58 ` Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).