[PATCH] Fix segmentation fault after opening an existing BFD for update PR gdb/20948

[Jozef Lawrynowicz <jozef dot l at mittosystems dot com>]

GDB segfaults when invoking it with the --write option, then quitting. First
reported in PR gdb/20948.

When GDB is invoked with the --write option, bfd_fopen is called with the "r+b"
flags (see comment #3 from the above PR for more info). A reduced test case
for the segfault using BFD only is below:

bfdbug.c
---
#include<bfd.h>

int main(int argc, char **argv)
{
  bfd *abfd = bfd_fopen(argv[1], "default", "r+", -1);
  bfd_check_format(abfd, bfd_object);
  bfd_close(abfd);
}
---

gcc bfdbug.c -g -static ./build/bfd/libbfd.a ./build/libiberty/libiberty.a -lopcodes -lz -ldl -o bfdbug
cp bfdbug test.elf
./build/gdb/gdb -ex run -ex quit --args ./bfdbug test.elf

Program received signal SIGSEGV, Segmentation fault.
_bfd_elf_strtab_finalize (tab=0x0) at ../../bfd/elf-strtab.c:371

---

The attached patch fixes the above seg fault, and therefore the GDB segfault
with --write, by initializing elf_shstrtab(bfd) when a bfd is opened for update.

Before writing elf_shstrtab and other section data back to the BFD, the filepos
of all the sections has to be recomputed. This is done in a new function,
_bfd_elf_recompute_section_file_positions, which takes the functionality
from _bfd_elf_compute_section_file_positions that is required to ensure the ELF
data is written back correctly.

With this patch, you can now, for example, get a section, modify the contents
and see those changes in the output BFD. After adding the following
to the example code above, we can see the string FOOBAR in the
"objdump -sj .data test.elf" output:

asection *section = bfd_get_section_by_name (abfd, ".data");
/* In ASCII:        "F"   "O"   "O"   "B"   "A"   "R"    */
char contents[] = {0x46, 0x4F, 0x4F, 0x42, 0x41, 0x52};
bfd_set_section_contents(abfd, section, contents, (file_ptr)0, sizeof(contents));

However, since output_has_begun is TRUE when the bfd is opened
for update, changes cannot be made to the size or order of sections, among
other things.

Tested gas, binutils, ld, gdb with no regressions for x86_64-pc-linux-gnu.
The "readelf -Ss" output is identical for both the before (bfdbug) and after
(test.elf) ELF files, indicating that the .strtab, .shstrtab and .symtab have
been read and written back correctly.

I couldn't work out where the above testcase would fit into the binutils
testsuite, and figured the build process for the testcase might be awkward
given the reliance on library calls. I've instead included a GDB test which
uses the --write functionality, and checks that the changes to memory in the
loaded ELF file persist after quitting GDB.

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

>From 647246418d1ecdc65a2bbdf45d80e792b7ce0432 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Fri, 16 Mar 2018 20:03:19 +0000
Subject: [PATCH] Fix segmentation fault after opening an existing BFD for
 update PR gdb/20948

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

Fix segmentation fault after opening an existing BFD for update PR gdb/20948

bfd/
  * elf.c (_bfd_elf_recompute_section_file_positions): New function.
  (_bfd_elf_write_object_contents): Call 
  _bfd_elf_recompute_section_file_positions if
  abfd->direction == both_direction.
  * elfcode.h (elf_object_p): Initialize elf_shstrtab(bfd) with the existing BFD
  data if abfd->direction == both_direction.

gdb/testsuite/gdb.base/
  * write_mem.exp: New test.
  * write_mem.c: New test source file.

---
 bfd/elf.c                            | 49 ++++++++++++++++++++++++++++
 bfd/elfcode.h                        | 62 ++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/write_mem.c   |  7 ++++
 gdb/testsuite/gdb.base/write_mem.exp | 34 ++++++++++++++++++++
 4 files changed, 152 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 8ea5a81..e1287a0 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -6355,6 +6355,52 @@ _bfd_elf_assign_file_positions_for_non_load (bfd *abfd)
   return TRUE;
 }
 
+/* Required when an existing BFD has been opened for update.
+   _bfd_elf_compute_section_file_positions does too much processing which
+   causes problems for existing BFDs.  */
+static bfd_boolean
+_bfd_elf_recompute_section_file_positions (bfd *abfd)
+{
+  Elf_Internal_Shdr **shdrpp, **end_shdrpp;
+  Elf_Internal_Shdr *shdrp;
+  file_ptr off;
+
+  /* When opening for update, no data has been explicitly written to the output
+     BFD yet.  */
+  BFD_ASSERT (elf_next_file_pos (abfd) == 0);
+
+  if (!assign_file_positions_for_load_sections (abfd, NULL))
+    return FALSE;
+
+  /* assign_file_positions_for_non_load_sections won't do anything if filepos
+     != 0, so set filepos to 0 for all non-load sections.  */
+  shdrpp = elf_elfsections (abfd);
+  end_shdrpp = shdrpp + elf_numsections (abfd);
+  for (shdrpp++; shdrpp < end_shdrpp; shdrpp++)
+    {
+      shdrp = *shdrpp;
+      if ((shdrp->sh_flags & SHF_ALLOC) == 0
+	  && shdrp->bfd_section != NULL)
+	shdrp->bfd_section->filepos = 0;
+    }
+  if (!assign_file_positions_for_non_load_sections (abfd, NULL))
+    return FALSE;
+
+  /* Upon exit from this function, shstrtab will be emitted, so set the
+     filepos of symtab and strtab now.  */
+  off = elf_next_file_pos (abfd);
+
+  shdrp = & elf_symtab_hdr (abfd);
+  off = _bfd_elf_assign_file_position_for_section (shdrp, off, TRUE);
+
+  shdrp = &elf_tdata (abfd)->strtab_hdr;
+  off = _bfd_elf_assign_file_position_for_section (shdrp, off, TRUE);
+
+  elf_next_file_pos (abfd) = off;
+
+  return TRUE;
+}
+
 bfd_boolean
 _bfd_elf_write_object_contents (bfd *abfd)
 {
@@ -6367,6 +6413,9 @@ _bfd_elf_write_object_contents (bfd *abfd)
   if (! abfd->output_has_begun
       && ! _bfd_elf_compute_section_file_positions (abfd, NULL))
     return FALSE;
+  else if (abfd->direction == both_direction
+	   && !_bfd_elf_recompute_section_file_positions (abfd))
+    return FALSE;
 
   i_shdrp = elf_elfsections (abfd);
 
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index fb02e25..e9f9e54 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -818,6 +818,68 @@ elf_object_p (bfd *abfd)
 	goto got_wrong_format_error;
     }
 
+  /* Now that the section headers have been processed, we can initialise the
+     internal section header string table.
+     This is only needed when opening the bfd for update because there is no
+     table to initialise for write_direction, and for read_direction this
+     internal structure is not needed as we never write out the shstrtab.  */
+  if (abfd->direction == both_direction)
+    {
+      Elf_Internal_Shdr **shdrpp, **end_shdrpp;
+      Elf_Internal_Shdr *shdrp;
+      struct elf_strtab_hash *shstrtab;
+      unsigned int count = 1;
+
+      shstrtab = _bfd_elf_strtab_init ();
+      BFD_ASSERT (shstrtab);
+      elf_shstrtab (abfd) = shstrtab;
+
+      shdrpp = elf_elfsections (abfd);
+      end_shdrpp = shdrpp + elf_numsections (abfd);
+
+      for (shdrpp++; shdrpp < end_shdrpp; shdrpp++)
+	{
+	  shdrp = *shdrpp;
+	  if (shdrp->bfd_section != NULL)
+	    _bfd_elf_strtab_add (shstrtab, shdrp->bfd_section->name, FALSE);
+	  else if (shdrp == &elf_tdata (abfd)->shstrtab_hdr)
+	    _bfd_elf_strtab_add (shstrtab, ".shstrtab", FALSE);
+	  else if (shdrp == &elf_tdata (abfd)->strtab_hdr)
+	    _bfd_elf_strtab_add (shstrtab, ".strtab", FALSE);
+	  else if (shdrp == &elf_tdata (abfd)->symtab_hdr)
+	    _bfd_elf_strtab_add (shstrtab, ".symtab", FALSE);
+	  else
+	    continue;
+	  /* At this early stage in the processing of the BFD, sh_name refers to
+	     the offset into the internal shstrtab hash table which is indexed
+	     in increments of 1.
+	     In _bfd_elf_write_object_contents, sh_name gets set to its final
+	     value which is the actual byte offset into the .shstrtab section.
+	     _bfd_elf_strtab_add returns this byte offset of sh_name so
+	     we can't use that return value at this stage.  */
+	  shdrp->sh_name = count;
+	  count++;
+	}
+      elf_strtab_sec (abfd) = elf_tdata (abfd)->strtab_hdr.sh_name;
+      elf_shstrtab_sec (abfd) = elf_tdata (abfd)->shstrtab_hdr.sh_name;
+      elf_onesymtab (abfd) = elf_tdata (abfd)->symtab_hdr.sh_name;
+
+      Elf_Internal_Shdr *shstrtab_hdr;
+      const struct elf_backend_data *bed = get_elf_backend_data (abfd);
+      shstrtab_hdr = &elf_tdata (abfd)->shstrtab_hdr;
+      shstrtab_hdr->sh_type = SHT_STRTAB;
+      shstrtab_hdr->sh_flags = bed->elf_strtab_flags;
+      shstrtab_hdr->sh_addr = 0;
+      /* sh_size is set in _bfd_elf_assign_file_positions_for_non_load.  */
+      shstrtab_hdr->sh_entsize = 0;
+      shstrtab_hdr->sh_link = 0;
+      shstrtab_hdr->sh_info = 0;
+      /* sh_offset is set in _bfd_elf_assign_file_positions_for_non_load.  */
+      shstrtab_hdr->sh_addralign = 1;
+
+      (*bed->elf_backend_post_process_headers) (abfd, NULL);
+    }
+
   /* Remember the entry point specified in the ELF file header.  */
   bfd_set_start_address (abfd, i_ehdrp->e_entry);
 
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..6c68b4e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/write_mem.exp
@@ -0,0 +1,34 @@
+# Test for PR gdb/20948
+# Verify that invoking gdb with the --write argument works as expected
+
+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