[patch] .gdb_index: Do not crash on NOBITS

[patch] .gdb_index: Do not crash on NOBITS

[Jan Kratochvil]

Hi,

elfutils-0.148 still do not contain patch of its GIT
804e9ca4d644e64a6125307cbf0a0b89477d7611 where the .gdb_index section has been
also split into the separate debug info file.

Due to it binaries split using elfutils-0.148 contain
  [38] .gdb_index        NOBITS           0000000000000000  0000338c
instead of expected
  [28] .gdb_index        PROGBITS         0000000000000000  0000211c

and due to it GDB while reading the file can error() by:
Reading symbols from x.debug...Dwarf Error: Can't read DWARF data from 'x.debug'

which should not be fatal but due to some other bugs therein it can crash GDB.

The wrong separate debug info file is for example:
http://kojipkgs.fedoraproject.org/packages/glibc/2.12.90/10/x86_64/glibc-debuginfo-2.12.90-10.x86_64.rpm
/usr/lib/debug/lib64/libutil-2.12.90.so.debug

OK to check-in?

It does not attempt to use .gdb_index from the main binary, it will just
disable .gdb_index usage on these binaries.


Thanks,
Jan


gdb/
2010-09-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c (dwarf2_read_index): Return on no SEC_HAS_CONTENTS.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1904,6 +1904,13 @@ dwarf2_read_index (struct objfile *objfile)
   if (dwarf2_per_objfile->gdb_index.asection == NULL
       || dwarf2_per_objfile->gdb_index.size == 0)
     return 0;
+
+  /* Older elfutils strip versions could keep the section in the main
+     executable while splitting it for the separate debug info file.  */
+  if ((bfd_get_file_flags (dwarf2_per_objfile->gdb_index.asection)
+       & SEC_HAS_CONTENTS) == 0)
+    return 0;
+
   dwarf2_read_section (objfile, &dwarf2_per_objfile->gdb_index);
 
   addr = dwarf2_per_objfile->gdb_index.buffer;

Re: [patch] .gdb_index: Do not crash on NOBITS

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> and due to it GDB while reading the file can error() by:
Jan> Reading symbols from x.debug...Dwarf Error: Can't read DWARF data from 'x.debug'
Jan> which should not be fatal but due to some other bugs therein it can
Jan> crash GDB.

I am curious about these other bugs.

Jan> OK to check-in?

Jan> It does not attempt to use .gdb_index from the main binary, it will just
Jan> disable .gdb_index usage on these binaries.

This is ok.  Thank you.

Tom

Re: [patch] .gdb_index: Do not crash on NOBITS

[Jan Kratochvil]

On Wed, 08 Sep 2010 21:40:12 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> and due to it GDB while reading the file can error() by:
> Jan> Reading symbols from x.debug...Dwarf Error: Can't read DWARF data from 'x.debug'
> Jan> which should not be fatal but due to some other bugs therein it can
> Jan> crash GDB.
> 
> I am curious about these other bugs.

Going to check it.


> This is ok.  Thank you.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-09/msg00062.html


Thanks,
Jan

[patch] Fix ELF stale reference [Re: [patch] .gdb_index: Do not crash on NOBITS]

[Jan Kratochvil]

On Wed, 08 Sep 2010 21:40:12 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> which should not be fatal but due to some other bugs therein it can
> Jan> crash GDB.
> 
> I am curious about these other bugs.

+      /* Memory gets permanently referenced from ABFD after
+	 bfd_get_synthetic_symtab so it must not get freed before ABFD gets.
+	 It happens only in the case when elf_slurp_reloc_table sees
+	 asection->relocation NULL.  Determining which section is asection is
+	 done by _bfd_elf_get_synthetic_symtab which is all a bfd
+	 implementation detail, though.  */

That is from:

#0  in elf_slurp_reloc_table_from_section (abfd, asect, rel_hdr, reloc_count=1170, relents, symbols, dynamic=1) at elfcode.h:1482
#1  in bfd_elf64_slurp_reloc_table (abfd, asect, symbols, dynamic=1) at elfcode.h:1563
#2  in _bfd_elf_get_synthetic_symtab (abfd, symcount=0, syms, dynsymcount=1792, dynsyms, ret) at elf.c:9269
#3  in elf_symfile_read (objfile, symfile_flags=6) at elfread.c:809

Where
	  elfcode.h:elf_slurp_reloc_table_from_section
contains
          ps = symbols + ELF_R_SYM (rela.r_info) - 1;
          relent->sym_ptr_ptr = ps;

`symbols' here is elf_symfile_read's `dyn_symbol_table'.  `dyn_symbol_table'
got immediately xfree'd but the freed memory remained referenced by
asect->relocation (containing the RELENT memory above, stored there by
elf_slurp_reloc_table).

asect->relocation probably does not get used if ABFD is not being read-in the
second time, which happens only if OBJFILE is being created the second time,
which happens due to the error call in the previous mail.


I was curious there elf_symfile_read uses 0 for COPY_NAMES in a similar case:
      elf_symtab_read (objfile, ST_REGULAR, symcount, symbol_table, 0);
where SYMBOL_TABLE is also immediately xfreed.  But that seems to be correct as
elf_slurp_symbol_table uses 
      symbase = (elf_symbol_type *) bfd_zalloc (abfd, amt);
for the content where later elfread.c's SYMBOL_TABLE points to.  Only the
pointers get xfreed which is OK.


No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.


Thanks,
Jan


gdb/
2010-09-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix stale memory references.
	* elfread.c: Include libbfd.h.
	(elf_symfile_read): Replace xmalloc by bfd_alloc, drop xfree, new
	comment.

--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -37,6 +37,7 @@
 #include "complaints.h"
 #include "demangle.h"
 #include "psympriv.h"
+#include "libbfd.h"
 
 extern void _initialize_elfread (void);
 
@@ -792,8 +793,14 @@ elf_symfile_read (struct objfile *objfile, int symfile_flags)
 
   if (storage_needed > 0)
     {
-      dyn_symbol_table = (asymbol **) xmalloc (storage_needed);
-      make_cleanup (xfree, dyn_symbol_table);
+      /* Memory gets permanently referenced from ABFD after
+	 bfd_get_synthetic_symtab so it must not get freed before ABFD gets.
+	 It happens only in the case when elf_slurp_reloc_table sees
+	 asection->relocation NULL.  Determining which section is asection is
+	 done by _bfd_elf_get_synthetic_symtab which is all a bfd
+	 implementation detail, though.  */
+
+      dyn_symbol_table = bfd_alloc (abfd, storage_needed);
       dynsymcount = bfd_canonicalize_dynamic_symtab (objfile->obfd,
 						     dyn_symbol_table);
 

Re: [patch] Fix ELF stale reference [Re: [patch] .gdb_index: Do not crash on NOBITS]

[Doug Evans]

On Thu, Sep 9, 2010 at 2:05 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
>
> gdb/
> 2010-09-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        Fix stale memory references.
>        * elfread.c: Include libbfd.h.
>        (elf_symfile_read): Replace xmalloc by bfd_alloc, drop xfree, new
>        comment.
>
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -37,6 +37,7 @@
>  #include "complaints.h"
>  #include "demangle.h"
>  #include "psympriv.h"
> +#include "libbfd.h"

Apologies for nitpicking but this raises an issue I'd like to understand better.
I thought libbfd.h was an internal bfd header.
[I know I've wanted to use it at least once and been told "No." :-)]

>
>  extern void _initialize_elfread (void);
>
> @@ -792,8 +793,14 @@ elf_symfile_read (struct objfile *objfile, int symfile_flags)
>
>   if (storage_needed > 0)
>     {
> -      dyn_symbol_table = (asymbol **) xmalloc (storage_needed);
> -      make_cleanup (xfree, dyn_symbol_table);
> +      /* Memory gets permanently referenced from ABFD after
> +        bfd_get_synthetic_symtab so it must not get freed before ABFD gets.
> +        It happens only in the case when elf_slurp_reloc_table sees
> +        asection->relocation NULL.  Determining which section is asection is
> +        done by _bfd_elf_get_synthetic_symtab which is all a bfd
> +        implementation detail, though.  */
> +
> +      dyn_symbol_table = bfd_alloc (abfd, storage_needed);
>       dynsymcount = bfd_canonicalize_dynamic_symtab (objfile->obfd,
>                                                     dyn_symbol_table);
>

Re: [patch] Fix ELF stale reference [Re: [patch] .gdb_index: Do not crash on NOBITS]

[Jan Kratochvil]

On Thu, 09 Sep 2010 16:51:51 +0200, Doug Evans wrote:
> On Thu, Sep 9, 2010 at 2:05 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > --- a/gdb/elfread.c
> > +++ b/gdb/elfread.c
> > @@ -37,6 +37,7 @@
> > +#include "libbfd.h"
> 
> Apologies for nitpicking but this raises an issue I'd like to understand better.
> I thought libbfd.h was an internal bfd header.
> [I know I've wanted to use it at least once and been told "No." :-)]

I have seen

amd64-darwin-tdep.c:#include "libbfd.h"
i386-darwin-tdep.c:#include "libbfd.h"
rs6000-nat.c:#include "libbfd.h"		/* For bfd_default_set_arch_mach (FIXME) */
rs6000-tdep.c:#include "libbfd.h"		/* for bfd_default_set_arch_mach */

so I considered it legal.

OTOH this patch is not completely clean, it can needlessly allocate
bfd-associated memory and the right fix would probably span into bfd/ IMO.


Thanks,
Jan

Re: [patch] Fix ELF stale reference

[Jan Kratochvil]

Hi,

I was debugging https://bugzilla.redhat.com/show_bug.cgi?id=642879 and got to
this fix from a different side.  It is in fact a very common GDB crash - due
to CTRL-C hit (to get GDB prompt) in the moment an ELF file is being read in.

Original thread: http://sourceware.org/ml/gdb-patches/2010-09/msg00192.html


On Thu, 09 Sep 2010 16:56:15 +0200, Jan Kratochvil wrote:
> OTOH this patch is not completely clean, it can needlessly allocate
> bfd-associated memory and the right fix would probably span into bfd/ IMO.

While the memory could use for example register_objfile_data_with_cleanup
instead of bfd_alloc so that if errors/CTRL-Cs happen the dynamic symbol table
pointers memory is not allocated twice.   Still I would not find it correct as
such memory would be objfile-bound instead of abfd-bound - while being
referenced by abfd.

OK to check-in?  Or some bfd/ API improvement should be made?


Thanks,
Jan


gdb/
2010-09-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix stale memory references.
	* elfread.c: Include libbfd.h.
	(elf_symfile_read): Replace xmalloc by bfd_alloc, drop xfree, new
	comment.

--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -37,6 +37,7 @@
 #include "complaints.h"
 #include "demangle.h"
 #include "psympriv.h"
+#include "libbfd.h"
 
 extern void _initialize_elfread (void);
 
@@ -792,8 +793,14 @@ elf_symfile_read (struct objfile *objfile, int symfile_flags)
 
   if (storage_needed > 0)
     {
-      dyn_symbol_table = (asymbol **) xmalloc (storage_needed);
-      make_cleanup (xfree, dyn_symbol_table);
+      /* Memory gets permanently referenced from ABFD after
+	 bfd_get_synthetic_symtab so it must not get freed before ABFD gets.
+	 It happens only in the case when elf_slurp_reloc_table sees
+	 asection->relocation NULL.  Determining which section is asection is
+	 done by _bfd_elf_get_synthetic_symtab which is all a bfd
+	 implementation detail, though.  */
+
+      dyn_symbol_table = bfd_alloc (abfd, storage_needed);
       dynsymcount = bfd_canonicalize_dynamic_symtab (objfile->obfd,
 						     dyn_symbol_table);
 

Re: [patch] Fix ELF stale reference

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> OK to check-in?  Or some bfd/ API improvement should be made?

I think this is probably the cleanest fix.

However, libbfd.h does say right at the top that it shouldn't be used.

I guess we could ask for advice on the binutils list.
Perhaps we could make bfd_alloc a public function.
Or maybe it is ok to bend this rule.

Tom

Re: [patch] Fix ELF stale reference

[Jan Kratochvil]

On Thu, 14 Oct 2010 19:46:13 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> OK to check-in?  Or some bfd/ API improvement should be made?
> 
> I think this is probably the cleanest fix.

Checked in.


> However, libbfd.h does say right at the top that it shouldn't be used.
> 
> I guess we could ask for advice on the binutils list.

This part has been fixed recently in binutils (as you forwarded me):
	http://sourceware.org/ml/binutils/2010-10/msg00413.html

So no new libbfd.h include is now needed in GDB.


Regarding the introduced memory leak it seems to be fixable but I have only
filed GDB PR for it now:
	http://sourceware.org/bugzilla/show_bug.cgi?id=12243


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-11/msg00094.html

--- src/gdb/ChangeLog	2010/11/19 18:10:43	1.12319
+++ src/gdb/ChangeLog	2010/11/19 22:30:44	1.12320
@@ -1,4 +1,10 @@
 2010-11-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix stale memory references.
+	* elfread.c (elf_symfile_read): Replace xmalloc by bfd_alloc, drop
+	xfree, new comment.
+
+2010-11-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
 	    Tom Tromey  <tromey@redhat.com>
 
 	* Makefile.in (.y.c): Directly create $@ from YLWRAP.
--- src/gdb/elfread.c	2010/10/01 20:26:11	1.99
+++ src/gdb/elfread.c	2010/11/19 22:30:47	1.100
@@ -790,8 +790,14 @@
 
   if (storage_needed > 0)
     {
-      dyn_symbol_table = (asymbol **) xmalloc (storage_needed);
-      make_cleanup (xfree, dyn_symbol_table);
+      /* Memory gets permanently referenced from ABFD after
+	 bfd_get_synthetic_symtab so it must not get freed before ABFD gets.
+	 It happens only in the case when elf_slurp_reloc_table sees
+	 asection->relocation NULL.  Determining which section is asection is
+	 done by _bfd_elf_get_synthetic_symtab which is all a bfd
+	 implementation detail, though.  */
+
+      dyn_symbol_table = bfd_alloc (abfd, storage_needed);
       dynsymcount = bfd_canonicalize_dynamic_symtab (objfile->obfd,
 						     dyn_symbol_table);