[patch] Fix crash on stale addrinfo->sectindex

[patch] Fix crash on stale addrinfo->sectindex

[Jan Kratochvil]

Hi,

bugreport by Robin Green:

https://bugzilla.redhat.com/show_bug.cgi?id=566128

==1846== Invalid write of size 4
==1846==    at 0x81788CC: relative_addr_info_to_section_offsets (symfile.c:561)
==1846==    by 0x808D29B: objfile_relocate (objfiles.c:907)
==1846==    by 0x80B823F: svr4_relocate_main_executable (solib-svr4.c:1734)
==1846==    by 0x80B82D6: svr4_solib_create_inferior_hook (solib-svr4.c:1810)
==1846==    by 0x80A29AA: solib_create_inferior_hook (solib.c:1039)

0x081788cc in relative_addr_info_to_section_offsets (section_offsets=0x9893ac0, num_sections=26, addrs=
    0x9892c08) at symfile.c:561
561	      section_offsets->offsets[osp->sectindex] = osp->addr;
(gdb) p osp->sectindex
$1 = 27
(gdb) p *addrs
$3 = {num_sections = 29, other = {{addr = 0, name = 0x9892e10 ".interp", sectindex = 0}}}
(gdb) up
#1  0x0808d29c in objfile_relocate (objfile=0x92b05d0, new_offsets=0xfef1bf60) at objfiles.c:907
907	      relative_addr_info_to_section_offsets (new_debug_offsets,
(gdb) p debug_objfile->num_sections 
$2 = 26
(gdb) down
#0  0x081788cc in relative_addr_info_to_section_offsets (section_offsets=0x9893ac0, num_sections=26, 
    addrs=0x9892c08) at symfile.c:561
561	      section_offsets->offsets[osp->sectindex] = osp->addr;
(gdb) p *addrs->other@addrs->num_sections 
$4 = {{addr = 0, name = 0x9892e10 ".interp", sectindex = 0}, {addr = 0, name = 0x9892e48 ".note.ABI-tag", 
    sectindex = 1}, {addr = 0, name = 0x9892e88 ".note.gnu.build-id", sectindex = 2}, {addr = 0, name = 
    0x9892ed0 ".gnu.hash", sectindex = 3}, {addr = 0, name = 0x9892f10 ".dynsym", sectindex = 4}, {
    addr = 0, name = 0x9892f48 ".gnu.liblist", sectindex = 5}, {addr = 0, name = 
    0x9892f88 ".gnu.version", sectindex = 6}, {addr = 0, name = 0x9892fc8 ".gnu.version_r", 
    sectindex = 7}, {addr = 0, name = 0x9893008 ".rel.dyn", sectindex = 8}, {addr = 0, name = 
    0x9893048 ".rel.plt", sectindex = 9}, {addr = 0, name = 0x9893088 ".init", sectindex = 10}, {
    addr = 0, name = 0x98930c0 ".plt", sectindex = 11}, {addr = 0, name = 0x98930f8 ".text", 
    sectindex = 12}, {addr = 0, name = 0x9893130 ".fini", sectindex = 13}, {addr = 0, name = 
    0x9893168 ".rodata", sectindex = 14}, {addr = 0, name = 0x98931a0 ".eh_frame_hdr", sectindex = 15}, {
    addr = 0, name = 0x98931e0 ".eh_frame", sectindex = 16}, {addr = 0, name = 0x9893220 ".ctors", 
    sectindex = 17}, {addr = 0, name = 0x9893258 ".dtors", sectindex = 18}, {addr = 0, name = 
    0x9893290 ".jcr", sectindex = 19}, {addr = 0, name = 0x98932c8 ".dynamic", sectindex = 20}, {
    addr = 0, name = 0x9893308 ".got", sectindex = 21}, {addr = 0, name = 0x9893340 ".got.plt", 
    sectindex = 22}, {addr = 0, name = 0x9893380 ".data", sectindex = 23}, {addr = 0, name = 
    0x98933b8 ".bss", sectindex = 24}, {addr = 5440, name = 0x98933f0 ".dynstr", sectindex = 5}, {
    addr = 0, name = 0x9893428 ".gnu.conflict", sectindex = 26}, {addr = 5440, name = 
    0x9893468 ".gnu_debuglink", sectindex = 27}, {addr = 5440, name = 0x98934a8 ".gnu.prelink_undo", 
    sectindex = 28}}

Initial investigation by Tom Tromey.

While it looks as caused by the objfile_relocate() part by me this problem
exists in GDB for a long time.  Curiously it is not reproducible for me on FSF
GDB HEAD but it is reproducible on gdb-7.0.50.20100203-15.fc13.  Did not try
more.

If there is no section with the given name it IMO does not make sense to try
setting any parameters for it.

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


Thanks,
Jan


2010-02-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (addr_info_make_relative): Extend comment.  Move SECT to
	a more inner block.  Initialize ADDR by LOWER_OFFSET only if it was
	found by bfd_get_section_by_name.
	* symfile.h (struct section_addr_info) <sectindex>: New comment.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -562,13 +562,13 @@ relative_addr_info_to_section_offsets (struct section_offsets *section_offsets,
 }
 
 /* Relativize absolute addresses in ADDRS into offsets based on ABFD.  Fill-in
-   also SECTINDEXes there.  */
+   also SECTINDEXes specific to ABFD there.  This function can be used to
+   rebase ADDRS to start referencing different BFD than before.  */
 
 void
 addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
 {
   asection *lower_sect;
-  asection *sect;
   CORE_ADDR lower_offset;
   int i;
 
@@ -597,25 +597,29 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
 
   for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
     {
-      if (addrs->other[i].addr != 0)
+      asection *sect = bfd_get_section_by_name (abfd, addrs->other[i].name);
+
+      if (sect)
 	{
-	  sect = bfd_get_section_by_name (abfd, addrs->other[i].name);
-	  if (sect)
+	  addrs->other[i].sectindex = sect->index;
+
+	  if (addrs->other[i].addr != 0)
 	    {
 	      addrs->other[i].addr -= bfd_section_vma (abfd, sect);
 	      lower_offset = addrs->other[i].addr;
 	      /* This is the index used by BFD. */
-	      addrs->other[i].sectindex = sect->index;
 	    }
 	  else
-	    {
-	      warning (_("section %s not found in %s"), addrs->other[i].name,
-		       bfd_get_filename (abfd));
-	      addrs->other[i].addr = 0;
-	    }
+	    addrs->other[i].addr = lower_offset;
 	}
       else
-	addrs->other[i].addr = lower_offset;
+	{
+	  warning (_("section %s not found in %s"), addrs->other[i].name,
+		   bfd_get_filename (abfd));
+	  addrs->other[i].addr = 0;
+
+	  /* SECTINDEX is invalid if ADDR is zero.  */
+	}
     }
 }
 
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -80,6 +80,8 @@ struct section_addr_info
   {
     CORE_ADDR addr;
     char *name;
+
+    /* SECTINDEX must be valid for associated BFD if ADDR is not zero.  */
     int sectindex;
   } other[1];
 };

Re: [patch] Fix crash on stale addrinfo->sectindex

[Tom Tromey]

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

Jan> Curiously it is not reproducible for me on FSF GDB HEAD but it is
Jan> reproducible on gdb-7.0.50.20100203-15.fc13.

Yes, odd.

Jan>  	      /* This is the index used by BFD. */
Jan> -	      addrs->other[i].sectindex = sect->index;

I think you ought to move this comment as well.
It appears to just be hanging at the end of a block after the patch.

This is ok with that change.  Thanks.

Tom

Re: [patch] Fix crash on stale addrinfo->sectindex

[Tom Tromey]

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

Jan> 2010-02-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* symfile.c (addr_info_make_relative): Extend comment.  Move SECT to
Jan> 	a more inner block.  Initialize ADDR by LOWER_OFFSET only if it was
Jan> 	found by bfd_get_section_by_name.
Jan> 	* symfile.h (struct section_addr_info) <sectindex>: New comment.

Oh, by the way, I think this is reasonable for 7.1.  It is
straightforward and fixes a reported crash.  If Joel agrees, please put
it on the branch.  Thanks.

Tom

Re: [patch] Fix crash on stale addrinfo->sectindex

[Jan Kratochvil]

On Fri, 19 Feb 2010 04:01:05 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> I think you ought to move this comment as well.
> It appears to just be hanging at the end of a block after the patch.

Yes; forgot.

> This is ok with that change.  Thanks.

Checked-in.

On Fri, 19 Feb 2010 04:10:30 +0100, Tom Tromey wrote:
> Oh, by the way, I think this is reasonable for 7.1.  It is
> straightforward and fixes a reported crash.  If Joel agrees, please put
> it on the branch.  Thanks.

OK for the branch?


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-02/msg00162.html

--- src/gdb/ChangeLog	2010/02/19 00:35:53	1.11380
+++ src/gdb/ChangeLog	2010/02/19 06:19:44	1.11381
@@ -1,3 +1,10 @@
+2010-02-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* symfile.c (addr_info_make_relative): Extend comment.  Move SECT to
+	a more inner block.  Initialize ADDR by LOWER_OFFSET only if it was
+	found by bfd_get_section_by_name.
+	* symfile.h (struct section_addr_info) <sectindex>: New comment.
+
 2010-02-19  Joel Brobecker  <brobecker@adacore.com>
 
 	* NEWS: Add new "[...] since 7.1" section.  Rename the "[...] since
--- src/gdb/symfile.c	2010/02/18 19:17:00	1.272
+++ src/gdb/symfile.c	2010/02/19 06:19:45	1.273
@@ -562,13 +562,13 @@
 }
 
 /* Relativize absolute addresses in ADDRS into offsets based on ABFD.  Fill-in
-   also SECTINDEXes there.  */
+   also SECTINDEXes specific to ABFD there.  This function can be used to
+   rebase ADDRS to start referencing different BFD than before.  */
 
 void
 addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
 {
   asection *lower_sect;
-  asection *sect;
   CORE_ADDR lower_offset;
   int i;
 
@@ -597,25 +597,29 @@
 
   for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
     {
-      if (addrs->other[i].addr != 0)
+      asection *sect = bfd_get_section_by_name (abfd, addrs->other[i].name);
+
+      if (sect)
 	{
-	  sect = bfd_get_section_by_name (abfd, addrs->other[i].name);
-	  if (sect)
+	  /* This is the index used by BFD. */
+	  addrs->other[i].sectindex = sect->index;
+
+	  if (addrs->other[i].addr != 0)
 	    {
 	      addrs->other[i].addr -= bfd_section_vma (abfd, sect);
 	      lower_offset = addrs->other[i].addr;
-	      /* This is the index used by BFD. */
-	      addrs->other[i].sectindex = sect->index;
 	    }
 	  else
-	    {
-	      warning (_("section %s not found in %s"), addrs->other[i].name,
-		       bfd_get_filename (abfd));
-	      addrs->other[i].addr = 0;
-	    }
+	    addrs->other[i].addr = lower_offset;
 	}
       else
-	addrs->other[i].addr = lower_offset;
+	{
+	  warning (_("section %s not found in %s"), addrs->other[i].name,
+		   bfd_get_filename (abfd));
+	  addrs->other[i].addr = 0;
+
+	  /* SECTINDEX is invalid if ADDR is zero.  */
+	}
     }
 }
 
--- src/gdb/symfile.h	2010/02/03 14:13:16	1.64
+++ src/gdb/symfile.h	2010/02/19 06:19:45	1.65
@@ -80,6 +80,8 @@
   {
     CORE_ADDR addr;
     char *name;
+
+    /* SECTINDEX must be valid for associated BFD if ADDR is not zero.  */
     int sectindex;
   } other[1];
 };

Re: [patch] Fix crash on stale addrinfo->sectindex

[Joel Brobecker]

> > Oh, by the way, I think this is reasonable for 7.1.  It is
> > straightforward and fixes a reported crash.  If Joel agrees, please put
> > it on the branch.  Thanks.
> 
> OK for the branch?

Absolutely - As usual, I am always happy to give an extra opinion, but
I think that GMs have enough knowledge and authority to approve patches
for the branch as well. I will look at all patches anyway, but no need
to wait for my approval.

-- 
Joel

Re: [patch] Fix crash on stale addrinfo->sectindex

[Jan Kratochvil]

On Fri, 19 Feb 2010 14:00:31 +0100, Joel Brobecker wrote:
> Absolutely - As usual, I am always happy to give an extra opinion, but
> I think that GMs have enough knowledge and authority to approve patches
> for the branch as well. I will look at all patches anyway, but no need
> to wait for my approval.

Checked-in on gdb_7_1-branch:
	http://sourceware.org/ml/gdb-cvs/2010-02/msg00163.html


Thanks,
Jan