Fix PR gdb/20948: --write option to GDB causes segmentation fault

Message ID de92821f-91d2-8058-bc51-2df7bed30191@mittosystems.com
State New
Headers show
Series
  • Fix PR gdb/20948: --write option to GDB causes segmentation fault
Related show

Commit Message

Jozef Lawrynowicz Sept. 13, 2018, 10:55 a.m.
When opening a BFD for update, as gdb --write does, modifications to
anything but the contents of sections is restricted.

Do not try to write back any ELF headers in this case.

Successfully tested with no regressions on x86_64-pc-linux-gnu with
the binutils, gas, ld and gdb testsuites.

The PR is assigned to GDB, but the fix is to BFD, hence the
submission to binutils mailing list instead of GDB.

Comments

Nick Clifton Sept. 17, 2018, 4:01 p.m. | #1
Hi Jozef,

> The PR is assigned to GDB, but the fix is to BFD, hence the

> submission to binutils mailing list instead of GDB.


The BFD part of your patch is approved.

Cheers
  Nick
Tom Tromey Sept. 17, 2018, 4:13 p.m. | #2
>>>>> "Jozef" == Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:


Jozef> The PR is assigned to GDB, but the fix is to BFD, hence the
Jozef> submission to binutils mailing list instead of GDB.

Jozef> 	* gdb/testsuite/gdb.base/write_mem.exp: New test.
Jozef> 	* gdb/testsuite/gdb.base/write_mem.c: Likewise.

The ChangeLog entries have to be split into the appropriate directories.

Jozef> diff --git a/gdb/testsuite/gdb.base/write_mem.c b/gdb/testsuite/gdb.base/write_mem.c
Jozef> new file mode 100644
Jozef> index 0000000..941f172
Jozef> --- /dev/null
Jozef> +++ b/gdb/testsuite/gdb.base/write_mem.c
Jozef> @@ -0,0 +1,7 @@
Jozef> +/* Test for PR gdb/20948.  */

Both new gdb files need the GPL header.
You can copy one from some other test file.

Jozef> +if {[build_executable $testfile.exp $testfile \
Jozef> +  $srcfile [list debug nowarnings] ] == -1} {

The indentation of this line looks off to me.
I would suggest checking other examples to see how it should look.

This is ok with these things fixed.  Thank you for the patch.

Tom
Jozef Lawrynowicz Sept. 23, 2018, 7:45 p.m. | #3
On 17/09/18 17:13, Tom Tromey wrote:
> Jozef> The PR is assigned to GDB, but the fix is to BFD, hence the

> Jozef> submission to binutils mailing list instead of GDB.

>

> Jozef> 	* gdb/testsuite/gdb.base/write_mem.exp: New test.

> Jozef> 	* gdb/testsuite/gdb.base/write_mem.c: Likewise.

>

> The ChangeLog entries have to be split into the appropriate directories.

>

> Jozef> diff --git a/gdb/testsuite/gdb.base/write_mem.c b/gdb/testsuite/gdb.base/write_mem.c

> Jozef> new file mode 100644

> Jozef> index 0000000..941f172

> Jozef> --- /dev/null

> Jozef> +++ b/gdb/testsuite/gdb.base/write_mem.c

> Jozef> @@ -0,0 +1,7 @@

> Jozef> +/* Test for PR gdb/20948.  */

>

> Both new gdb files need the GPL header.

> You can copy one from some other test file.

>

> Jozef> +if {[build_executable $testfile.exp $testfile \

> Jozef> +  $srcfile [list debug nowarnings] ] == -1} {

>

> The indentation of this line looks off to me.

> I would suggest checking other examples to see how it should look.

>

> This is ok with these things fixed.  Thank you for the patch.

>

> Tom


Thanks for the review, the updated patch is attached. I would appreciate 
it if someone would commit the patch for me, as I do not have write 
access. Jozef
From 24fee9b2ce3881285f1b0a2e1bab5a2aa1e034e4 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>

Date: Tue, 11 Sep 2018 22:56:36 +0100
Subject: [PATCH] Fix PR gdb/20948: --write option to GDB causes segmentation
 fault

When opening a BFD for update, as gdb --write does, modifications to
anything but the contents of sections is restricted.

Do not try to write back any ELF headers in this case.

bfd/ChangeLog:

2018-09-XX  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	PR gdb/20948
	* elf.c (_bfd_elf_write_object_contents): Return from function
	early if abfd->direction == both_direction.

gdb/testsuite/ChangeLog:

2018-09-XX  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	PR gdb/20948
	* gdb.base/write_mem.exp: New test.
	* gdb.base/write_mem.c: Likewise.
---
 bfd/elf.c                            | 12 +++++++++
 gdb/testsuite/gdb.base/write_mem.c   | 20 +++++++++++++++
 gdb/testsuite/gdb.base/write_mem.exp | 47 ++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 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 02d605c..5320ae2 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -6391,6 +6391,18 @@ _bfd_elf_write_object_contents (bfd *abfd)
   if (! abfd->output_has_begun
       && ! _bfd_elf_compute_section_file_positions (abfd, NULL))
     return FALSE;
+  /* Do not rewrite ELF data when the BFD has been opened for update.
+     abfd->output_has_begun was set to TRUE on opening, so creation of new
+     sections, and modification of existing section sizes was restricted.
+     This means the ELF header, program headers and section headers can't have
+     changed.
+     If the contents of any sections has been modified, then those changes have
+     already been written to the BFD.  */
+  else if (abfd->direction == both_direction)
+    {
+      BFD_ASSERT (abfd->output_has_begun);
+      return TRUE;
+    }
 
   i_shdrp = elf_elfsections (abfd);
 
diff --git a/gdb/testsuite/gdb.base/write_mem.c b/gdb/testsuite/gdb.base/write_mem.c
new file mode 100644
index 0000000..82d8c41
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.c
@@ -0,0 +1,20 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+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..db476e7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.exp
@@ -0,0 +1,47 @@
+# Copyright (C) 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Contributed by Jozef Lawrynowicz (jozef.l@mittosystems.com)
+
+# Test for PR gdb/20948
+# Verify that invoking gdb with the --write argument works as expected
+
+global GDBFLAGS
+standard_testfile
+
+if {[build_executable $testfile.exp $testfile \
+	$srcfile [list debug nowarnings] ] == -1} {
+    untested $testfile.exp
+    return -1
+}
+
+set old_gdbflags $GDBFLAGS
+
+# Expect a failure before --write has been added to the command line
+set GDBFLAGS "$old_gdbflags $binfile"
+clean_restart
+test_print_reject "set {int}main = 0x4242" "Cannot access memory at address"
+
+# Setting memory should now work correctly after adding --write
+set GDBFLAGS "$old_gdbflags --write $binfile"
+clean_restart
+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
Tom Tromey Sept. 24, 2018, 12:22 p.m. | #4
>>>>> "Jozef" == Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:


Jozef> Thanks for the review, the updated patch is attached. I would
Jozef> appreciate it if someone would commit the patch for me, as I do not
Jozef> have write access.

I've pushed it.  Thanks once again for the patch.

Tom

Patch

From b61a0bf98f7effdbee30f0a2b9dfe488b69edf54 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Tue, 11 Sep 2018 22:56:36 +0100
Subject: [PATCH] Fix PR gdb/20948: --write option to GDB causes segmentation
 fault

When opening a BFD for update, as gdb --write does, modifications to
anything but the contents of sections is restricted.

Do not try to write back any ELF headers in this case.

	PR gdb/20948
	* bfd/elf.c (_bfd_elf_write_object_contents): Return from function
	early if abfd->direction == both_direction.
	* gdb/testsuite/gdb.base/write_mem.exp: New test.
	* gdb/testsuite/gdb.base/write_mem.c: Likewise.
---
 bfd/elf.c                            | 12 ++++++++++++
 gdb/testsuite/gdb.base/write_mem.c   |  7 +++++++
 gdb/testsuite/gdb.base/write_mem.exp | 30 ++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
 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 02d605c..5320ae2 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -6391,6 +6391,18 @@  _bfd_elf_write_object_contents (bfd *abfd)
   if (! abfd->output_has_begun
       && ! _bfd_elf_compute_section_file_positions (abfd, NULL))
     return FALSE;
+  /* Do not rewrite ELF data when the BFD has been opened for update.
+     abfd->output_has_begun was set to TRUE on opening, so creation of new
+     sections, and modification of existing section sizes was restricted.
+     This means the ELF header, program headers and section headers can't have
+     changed.
+     If the contents of any sections has been modified, then those changes have
+     already been written to the BFD.  */
+  else if (abfd->direction == both_direction)
+    {
+      BFD_ASSERT (abfd->output_has_begun);
+      return TRUE;
+    }
 
   i_shdrp = elf_elfsections (abfd);
 
diff --git a/gdb/testsuite/gdb.base/write_mem.c b/gdb/testsuite/gdb.base/write_mem.c
new file mode 100644
index 0000000..941f172
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.c
@@ -0,0 +1,7 @@ 
+/* Test for PR gdb/20948.  */
+
+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..e1f393a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.exp
@@ -0,0 +1,30 @@ 
+# Test for PR gdb/20948
+# Verify that invoking gdb with the --write argument works as expected
+
+global GDBFLAGS
+standard_testfile
+
+if {[build_executable $testfile.exp $testfile \
+  $srcfile [list debug nowarnings] ] == -1} {
+    untested $testfile.exp
+    return -1
+}
+
+set old_gdbflags $GDBFLAGS
+
+# Expect a failure before --write has been added to the command line
+set GDBFLAGS "$old_gdbflags $binfile"
+clean_restart
+test_print_reject "set {int}main = 0x4242" "Cannot access memory at address"
+
+# Setting memory should now work correctly after adding --write
+set GDBFLAGS "$old_gdbflags --write $binfile"
+clean_restart
+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