diff mbox

Fix seg fault with --write PR gdb/20948

Message ID 845cbec4-aa1d-005e-b7f3-d010ecd8cf74@mittosystems.com
State New, archived
Headers

Commit Message

Jozef Lawrynowicz March 8, 2018, 10:25 p.m. UTC 
  GDB segfaults when invoking it with the --write option, then quitting. First
reported in PR gdb/20948.

An assertion fails because elf_shstrtab is uninitialized, and 
elf_shstrtab is
only initialized if abfd_output_has_begun is FALSE.
bfd/format.c:bfd_check_format_matches as called from 
gdb/exec.c:exec_file_attach
always sets output_has_begun to TRUE if the bfd was opened for update, 
so the
attached patch sets output_has_begun back to FALSE in exec_file_attach 
when we
return from bfd_check_format_matches.

This leads to a further assertion failure in
bfd/elf.c:assign_file_positions_for_non_load_sections:

BFD_ASSERT (hdr->sh_offset == hdr->bfd_section->filepos);

filepos for non-load sections has been set already, but sh_offset is 0 as it
needs to be set by _bfd_elf_assign_file_position_for_section, which is 
called in
a further conditional block. So this first conditional has been extended to
evaluate to FALSE if sh_offset == 0 but filepos != 0.

The attached patche includes tests which verify that the --write behaviour
works as expected i.e. that modifications to the loaded executable persist
once the GDB session is ended.

For Unix and msp430-elf targets, completed testing for binutils, gas, 
ld, gdb,
sim (for msp430) without regressions.

If the patch is acceptable, I would appreciate if someone could commit 
it for
me as I don't have write access.

Comments

Simon Marchi March 11, 2018, 4:47 p.m. UTC | #1 
On 2018-03-08 05:25 PM, Jozef Lawrynowicz wrote:
> GDB segfaults when invoking it with the --write option, then quitting. First
> reported in PR gdb/20948.
> 
> An assertion fails because elf_shstrtab is uninitialized, and 
> elf_shstrtab is
> only initialized if abfd_output_has_begun is FALSE.
> bfd/format.c:bfd_check_format_matches as called from 
> gdb/exec.c:exec_file_attach
> always sets output_has_begun to TRUE if the bfd was opened for update, 
> so the
> attached patch sets output_has_begun back to FALSE in exec_file_attach 
> when we
> return from bfd_check_format_matches.
> 
> This leads to a further assertion failure in
> bfd/elf.c:assign_file_positions_for_non_load_sections:
> 
> BFD_ASSERT (hdr->sh_offset == hdr->bfd_section->filepos);
> 
> filepos for non-load sections has been set already, but sh_offset is 0 as it
> needs to be set by _bfd_elf_assign_file_position_for_section, which is 
> called in
> a further conditional block. So this first conditional has been extended to
> evaluate to FALSE if sh_offset == 0 but filepos != 0.
> 
> The attached patche includes tests which verify that the --write behaviour
> works as expected i.e. that modifications to the loaded executable persist
> once the GDB session is ended.
> 
> For Unix and msp430-elf targets, completed testing for binutils, gas, 
> ld, gdb,
> sim (for msp430) without regressions.
> 
> If the patch is acceptable, I would appreciate if someone could commit 
> it for
> me as I don't have write access.
> 


Hi Jozef,

Thanks for looking into this.

Because of the change in bfd/, this patch should also be sent to the
binutils@sourceware.org mailing list.  The change we'll have to do
in GDB may depend on what is the final solution on the BFD side.

Also, is the problem illustrated in comment #3 of the bug report related?

https://sourceware.org/bugzilla/show_bug.cgi?id=20948#c3

I tried that snippet with your patch applied, and still get a segfault.
I think it would be good to get a fix for the minimal reproducing example
first.

When you post your patch to the binutils mailing list, you can also refer
to the gdb bug (PR20948) and CC the gdb-patches mailing list, so we can
follow the discussion.

Thanks!

Simon

Simon
Jozef Lawrynowicz March 13, 2018, 10:43 p.m. UTC | #2 
On 11/03/18 16:47, Simon Marchi wrote:
> Hi Jozef,
>
> Thanks for looking into this.
>
> Because of the change in bfd/, this patch should also be sent to the
> binutils@sourceware.org mailing list.  The change we'll have to do
> in GDB may depend on what is the final solution on the BFD side.
>
> Also, is the problem illustrated in comment #3 of the bug report related?
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=20948#c3
>
> I tried that snippet with your patch applied, and still get a segfault.
> I think it would be good to get a fix for the minimal reproducing example
> first.
>
> When you post your patch to the binutils mailing list, you can also refer
> to the gdb bug (PR20948) and CC the gdb-patches mailing list, so we can
> follow the discussion.
>
> Thanks!
>
> Simon
>
> Simon

Hi Simon,

I missed the example in comment #3, you are right there is a general issue with
BFD rather than this being GDB specific.

It seems that the internal representation of SHSTRTAB (elf_shstrtab(bfd)) does
not get initialized or populated when opening an existing BFD. This is not an
issue when only reading, as this specific internal representation is not
required. It is also not an issue (or at least doesn't cause any errors) when
only writing as a new shstrtab is created. But when reading and writing,
elf_shstrtab needs to be populated using the existing elf data in the loaded
BFD and this doesn't happen at the moment.

I'm working on a patch.

I reckon when the BFD issue is fixed, we may not need any specific patch for
GDB.

Thanks,
Jozef
Simon Marchi March 14, 2018, 9:03 p.m. UTC | #3 
On 2018-03-13 18:43, Jozef Lawrynowicz wrote:
> Hi Simon,
> 
> I missed the example in comment #3, you are right there is a general 
> issue with
> BFD rather than this being GDB specific.
> 
> It seems that the internal representation of SHSTRTAB 
> (elf_shstrtab(bfd)) does
> not get initialized or populated when opening an existing BFD. This is 
> not an
> issue when only reading, as this specific internal representation is 
> not
> required. It is also not an issue (or at least doesn't cause any 
> errors) when
> only writing as a new shstrtab is created. But when reading and 
> writing,
> elf_shstrtab needs to be populated using the existing elf data in the 
> loaded
> BFD and this doesn't happen at the moment.
> 
> I'm working on a patch.
> 
> I reckon when the BFD issue is fixed, we may not need any specific 
> patch for
> GDB.
> 
> Thanks,
> Jozef

Ok, thanks for looking into it.

Simon
diff mbox

Patch

From 5a9c30cac1edbdd8675025dc3faffa6e6544487d Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Wed, 7 Mar 2018 16:57:15 +0000
Subject: [PATCH] Fix GDB segfault with --write

2018-03-08  Jozef Lawrynowicz <jozef.l@mittosystems.com>

Fix PR gdb/20948

bfd/
  * elf.c (assign_file_positions_for_non_load_sections): Allow hdr->sh_offset
    to be set by function if hdr->bfd_section->filepos != 0.

gdb/
  * exec.c (exec_file_attach): Set output_has_begun to FALSE for the bfd if
    write_files is TRUE.

gdb/testsuite/gdb.base/
  * write_mem.exp: New test.
  * write_mem.c: New test source file.
---
 bfd/elf.c                            |  9 ++++++++-
 gdb/exec.c                           |  4 ++++
 gdb/testsuite/gdb.base/write_mem.c   |  5 +++++
 gdb/testsuite/gdb.base/write_mem.exp | 31 +++++++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/write_mem.c
 create mode 100644 gdb/testsuite/gdb.base/write_mem.exp

diff --git a/bfd/elf.c b/bfd/elf.c
index 8ea5a81..b731e90 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -5736,7 +5736,14 @@  assign_file_positions_for_non_load_sections (bfd *abfd,
 
       hdr = *hdrpp;
       if (hdr->bfd_section != NULL
-	  && (hdr->bfd_section->filepos != 0
+	  && (((hdr->sh_flags & SHF_ALLOC) != 0
+	       && hdr->bfd_section->filepos != 0)
+	      /* If we haven't been called by the ELF linker, non load sections
+		 may have filepos>0 but sh_offset==0, so allow this function
+		 to set sh_offset.  */
+	      || ((hdr->sh_flags & SHF_ALLOC) == 0
+		  && hdr->bfd_section->filepos != 0
+		  && hdr->sh_offset != 0)
 	      || (hdr->sh_type == SHT_NOBITS
 		  && hdr->contents == NULL)))
 	BFD_ASSERT (hdr->sh_offset == hdr->bfd_section->filepos);
diff --git a/gdb/exec.c b/gdb/exec.c
index 6b910e8..c6662e0 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -351,6 +351,10 @@  exec_file_attach (const char *filename, int from_tty)
 		 gdb_bfd_errmsg (bfd_get_error (), matching));
 	}
 
+      /* bfd_check_format_matches assumes output_has_begun in all cases.  */
+      if (write_files)
+	exec_bfd->output_has_begun = FALSE;
+
       if (build_section_table (exec_bfd, &sections, &sections_end))
 	{
 	  /* Make sure to close exec_bfd, or else "run" might try to use
diff --git a/gdb/testsuite/gdb.base/write_mem.c b/gdb/testsuite/gdb.base/write_mem.c
new file mode 100644
index 0000000..b72eacd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.c
@@ -0,0 +1,5 @@ 
+int main (void)
+{
+  while (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/write_mem.exp b/gdb/testsuite/gdb.base/write_mem.exp
new file mode 100644
index 0000000..558c61d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.exp
@@ -0,0 +1,31 @@ 
+global GDBFLAGS
+
+standard_testfile
+
+global srcdir
+global subdir
+
+if {[build_executable $testfile.exp $testfile \
+    $srcfile [list debug nowarnings] ] == -1} {
+	untested $testfile.exp
+	  return -1
+}
+
+clean_restart
+
+# Expect a failure before --write has been added to the command line
+test_print_reject "set {int}&main = 0x4242"
+
+set old_gdbflags $GDBFLAGS
+set GDBFLAGS "$old_gdbflags --write $binfile"
+clean_restart
+
+# Setting memory should now work correctly
+gdb_test_no_output "set {int}&main = 0x4242"
+
+# Check that memory write persists after quitting GDB
+gdb_exit
+gdb_start
+gdb_test "x /xh main" "<main>:.*4242"
+
+set GDBFLAGS $old_gdbflags
-- 
2.7.4