* 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).