[PATCH] Fix "gdb --write" with core files

[PATCH] Fix "gdb --write" with core files

[Pedro Alves]

If you load a core file into GDB with the --write option, or "set
write on" (equivalent), and then poke memory expecting it to patch the
core binary, you'll notice something odd -- the write seems to
succeed, but in reality, it doesn't.  The value you wrote doesn't
persist.  Like so:

 $ gdb -q --write -c testsuite/outputs/gdb.base/patch/gcore.test
 [New LWP 615986]
 Core was generated by `/home/pedro/gdb/build/gdb/testsuite/outputs/gdb.base/patch/patch'.
 Program terminated with signal SIGTRAP, Trace/breakpoint trap.
 #0  0x0000555555555131 in ?? ()
 (gdb) p *(unsigned char *)0x0000555555555131 = 1
 $1 = 1 '\001'
 (gdb) p *(unsigned char *)0x0000555555555131
 $2 = 185 '\271'
 (gdb)

Diffing hexdumps of before/after patching, reveals that a "0x1" was
actually written somewhere in the file.  The problem is that the "0x1"
was written at the wrong offset in the file...

That happens because _bfd_elf_set_section_contents does this to seek
to the section's offset:

  pos = hdr->sh_offset + offset;
  if (bfd_seek (abfd, pos, SEEK_SET) != 0
      || bfd_bwrite (location, count, abfd) != count)
    return false;

... and 'hdr->sh_offset' is zero, so we seek to just OFFSET, which is
incorrect.  The reason 'hdr->sh_offset' is zero is that
kernel-generated core files normally don't even have a section header
table (gdb-generated ones do, but that's more an accident than a
feature), and indeed elf_core_file_p doesn't even try to read sections
at all:

  /*  Core files are simply standard ELF formatted files that partition
      the file using the execution view of the file (program header table)
      rather than the linking view.  In fact, there is no section header
      table in a core file.

      The process status information (including the contents of the general
      register set) and the floating point register set are stored in a
      segment of type PT_NOTE.  We handcraft a couple of extra bfd sections
      that allow standard bfd access to the general registers (.reg) and the
      floating point registers (.reg2).  */

  bfd_cleanup
  elf_core_file_p (bfd *abfd)

My first approach was to fix it in _bfd_elf_set_section_contents
directly -- if bfd->direction is both_direction, and updating a core,
then jump straight to _bfd_generic_set_section_contents, which is
documented to work exactly in this scenario:

  /* This generic function can only be used in implementations where creating
     NEW sections is disallowed.  It is useful in patching existing sections
     in read-write files, though.  See other set_section_contents functions
     to see why it doesn't work for new sections.  */
  bool
  _bfd_generic_set_section_contents (bfd *abfd,

However, on second thought, it sounds like this is something that
might need to be done too for other formats and/or ELF backends if
they install custom versions of set_section_contents, or if when
opening they also create handcrafted BFD sections, so I thought it
might be better done in common code.  And thus that is what this patch
does.

New GDB testcase included, which exercises both patching an executable
and patching a core file.  Patching an executable already works
without this fix, because in that case BFD reads in the sections
table.  Still, we had no testcase for that yet.  In fact, we have no
"set write on" testcases at all, this is the first one.

Tested on x86-64 GNU/Linux, gdb, ld, binutils, and gas.

Change-Id: I0f49f58b48aabab2e269f2959b8fd8a7fe36fdce
---
 bfd/section.c                    |  26 ++++++++
 gdb/testsuite/gdb.base/patch.c   |  26 ++++++++
 gdb/testsuite/gdb.base/patch.exp | 106 +++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/patch.c
 create mode 100644 gdb/testsuite/gdb.base/patch.exp

diff --git a/bfd/section.c b/bfd/section.c
index 5a487ce6c6f..23d59385f5f 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -1507,6 +1507,24 @@ bfd_set_section_contents (bfd *abfd,
       && location != section->contents + offset)
     memcpy (section->contents + offset, location, (size_t) count);
 
+  /* When a BFD is opened for update, the backend's data structures
+     may not represent all the sections in the BFD.  E.g., when
+     reading ELF core files, we don't even read their section header
+     table (elf_core_file_p), while the ELF backend's
+     set_section_contents implementation assumes it can find a BFD
+     section's file offset from the interned ELF section table info.
+     Core files do end up with BFD sections after
+     open/bfd_check_format, but those sections are made up by BFD and
+     don't exist in the actual core file, like the "load" and ".reg"
+     sections.  To avoid potentially every backend having to handle
+     these scenarios, jump directly to the generic version here.  */
+  if (abfd->direction == both_direction)
+    {
+      BFD_ASSERT (abfd->output_has_begun);
+      return _bfd_generic_set_section_contents (abfd, section,
+						location, offset, count);
+    }
+
   if (BFD_SEND (abfd, _bfd_set_section_contents,
 		(abfd, section, location, offset, count)))
     {
@@ -1592,6 +1610,14 @@ bfd_get_section_contents (bfd *abfd,
       return true;
     }
 
+  /* For consistency with bfd_set_section_contents.  */
+  if (abfd->direction == both_direction)
+    {
+      BFD_ASSERT (abfd->output_has_begun);
+      return _bfd_generic_get_section_contents (abfd, section,
+						location, offset, count);
+    }
+
   return BFD_SEND (abfd, _bfd_get_section_contents,
 		   (abfd, section, location, offset, count));
 }
diff --git a/gdb/testsuite/gdb.base/patch.c b/gdb/testsuite/gdb.base/patch.c
new file mode 100644
index 00000000000..c75f7800c94
--- /dev/null
+++ b/gdb/testsuite/gdb.base/patch.c
@@ -0,0 +1,26 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+/* Value != 0 so it doesn't ends up in .bss, which we wouldn't be able
+   to patch.  */
+int extern_global = 1;
+
+int
+main()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/patch.exp b/gdb/testsuite/gdb.base/patch.exp
new file mode 100644
index 00000000000..6d9c17ff740
--- /dev/null
+++ b/gdb/testsuite/gdb.base/patch.exp
@@ -0,0 +1,106 @@
+# Copyright 2022 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/>.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Check that we can patch an executable.
+
+with_test_prefix "exec" {
+    clean_restart
+
+    gdb_test_no_output "set write on"
+
+    gdb_load $binfile
+
+    gdb_test "p extern_global" " = 1" "read original value"
+    gdb_test "p extern_global = 2" " = 2" "modify value"
+    gdb_test "p extern_global" " = 2" "value modified"
+
+    clean_restart $binfile
+
+    gdb_test "p extern_global" " = 2" "value modified persisted"
+}
+
+# Check that we can patch a core file.
+
+# Generate a core file.
+with_test_prefix "gcore" {
+    clean_restart $binfile
+
+    if {![runto_main]} {
+	return
+    }
+
+    # Extract the value at PC, and add 1, letting it wrap if
+    # necessary.  This is the value we will poke into memory.
+    set poke_value ""
+    gdb_test_multiple "p/x (*(unsigned char *) \$pc) + 1" "compute poke value" {
+	-re -wrap " = ($hex)" {
+	    set poke_value $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+    if {$poke_value == ""} {
+	return
+    }
+
+    set corefile [standard_output_file gcore.test]
+    set core_supported [gdb_gcore_cmd "$corefile" "save a corefile"]
+    if {!$core_supported} {
+	return
+    }
+}
+
+# Load it into GDB with "set write on", and change the instruction at
+# PC.
+with_test_prefix "load core, write on" {
+    # Don't load a binary, we want to make sure we're patching the
+    # core, not the executable.
+    clean_restart
+
+    gdb_test_no_output "set write on"
+
+    set core_loaded [gdb_core_cmd "$corefile" "re-load generated corefile"]
+    if { $core_loaded == -1 } {
+	# No use proceeding from here.
+	return
+    }
+
+    gdb_test "p/x *(char *) \$pc = $poke_value" \
+	" = $poke_value" \
+	"poke value"
+    gdb_test "p/x *(char *) \$pc" \
+	" = $poke_value" \
+	"value modified"
+}
+
+# Re-load it into a new GDB session, now with "set write off", and
+# confirm the value change persisted.
+with_test_prefix "re-load modified core" {
+    # Don't load a binary, we want to make sure we've patched the
+    # core, not the executable.
+    clean_restart
+
+    set core_loaded [gdb_core_cmd "$corefile" "re-load generated corefile"]
+    gdb_assert { $core_loaded != -1 } "core re-loaded"
+
+    gdb_test "p/x *(char *) \$pc" \
+	" = $poke_value" \
+	"value modified persisted"
+}

base-commit: c8a9e88bf6ff32d90d082d07d3c5d12b938f8335
-- 
2.36.0

Re: [PATCH] Fix "gdb --write" with core files

[Alan Modra]

On Thu, May 12, 2022 at 03:49:15PM +0100, Pedro Alves wrote:
> That happens because _bfd_elf_set_section_contents does this to seek
> to the section's offset:
> 
>   pos = hdr->sh_offset + offset;
>   if (bfd_seek (abfd, pos, SEEK_SET) != 0
>       || bfd_bwrite (location, count, abfd) != count)
>     return false;
> 
> ... and 'hdr->sh_offset' is zero, so we seek to just OFFSET, which is
> incorrect.  The reason 'hdr->sh_offset' is zero is that
> kernel-generated core files normally don't even have a section header
> table (gdb-generated ones do, but that's more an accident than a
> feature), and indeed elf_core_file_p doesn't even try to read sections
> at all:

I'm a little concerned about your patch keying off both_direction.  In
particular it seems odd to change bfd behaviour when reading depending
on whether the file was opened "r" or "r+".

Did you try changing
  pos = hdr->sh_offset + offset;
to
  pos = section->filepos + offset;
in _bfd_elf_set_section_contents?  I think that should work for you,
and not break ELF targets.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Fix "gdb --write" with core files

[Pedro Alves]

On 2022-05-13 03:57, Alan Modra wrote:
> On Thu, May 12, 2022 at 03:49:15PM +0100, Pedro Alves wrote:
>> That happens because _bfd_elf_set_section_contents does this to seek
>> to the section's offset:
>>
>>   pos = hdr->sh_offset + offset;
>>   if (bfd_seek (abfd, pos, SEEK_SET) != 0
>>       || bfd_bwrite (location, count, abfd) != count)
>>     return false;
>>
>> ... and 'hdr->sh_offset' is zero, so we seek to just OFFSET, which is
>> incorrect.  The reason 'hdr->sh_offset' is zero is that
>> kernel-generated core files normally don't even have a section header
>> table (gdb-generated ones do, but that's more an accident than a
>> feature), and indeed elf_core_file_p doesn't even try to read sections
>> at all:
> 
> I'm a little concerned about your patch keying off both_direction.  In
> particular it seems odd to change bfd behaviour when reading depending
> on whether the file was opened "r" or "r+".

Fair point.

> 
> Did you try changing
>   pos = hdr->sh_offset + offset;
> to
>   pos = section->filepos + offset;
> in _bfd_elf_set_section_contents?  I think that should work for you,
> and not break ELF targets.
> 

Ah, I had assumed using hdr->sh_offset was important in write_direction
somehow.

That works, and then we could just defer to _bfd_generic_set_section_contents,
which does exactly the same thing.  It's like as if in C++ you override a method
to do extra things, and then call the base class's implementation.

WDYT of this version, then?

I've retested gas/binutils/ld/gdb on x86_64 GNU/Linux.

Thanks!

From a9935ffa786d4aefd7bd12f4ad23ace87ad5d4a1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 11 May 2022 14:20:15 +0100
Subject: [PATCH] Fix "gdb --write" with core files

If you load a core file into GDB with the --write option, or "set
write on" (equivalent), and then poke memory expecting it to patch the
core binary, you'll notice something odd -- the write seems to
succeed, but in reality, it doesn't.  The value you wrote doesn't
persist.  Like so:

 $ gdb -q --write -c testsuite/outputs/gdb.base/patch/gcore.test
 [New LWP 615986]
 Core was generated by `/home/pedro/gdb/build/gdb/testsuite/outputs/gdb.base/patch/patch'.
 Program terminated with signal SIGTRAP, Trace/breakpoint trap.
 #0  0x0000555555555131 in ?? ()
 (gdb) p *(unsigned char *)0x0000555555555131 = 1
 $1 = 1 '\001'
 (gdb) p *(unsigned char *)0x0000555555555131
 $2 = 185 '\271'
 (gdb)

Diffing hexdumps of before/after patching, reveals that a "0x1" was
actually written somewhere in the file.  The problem is that the "0x1"
was written at the wrong offset in the file...

That happens because _bfd_elf_set_section_contents does this to seek
to the section's offset:

  pos = hdr->sh_offset + offset;
  if (bfd_seek (abfd, pos, SEEK_SET) != 0
      || bfd_bwrite (location, count, abfd) != count)
    return false;

... and 'hdr->sh_offset' is zero, so we seek to just OFFSET, which is
incorrect.  The reason 'hdr->sh_offset' is zero is that
kernel-generated core files normally don't even have a section header
table (gdb-generated ones do, but that's more an accident than a
feature), and indeed elf_core_file_p doesn't even try to read sections
at all:

  /*  Core files are simply standard ELF formatted files that partition
      the file using the execution view of the file (program header table)
      rather than the linking view.  In fact, there is no section header
      table in a core file.

      The process status information (including the contents of the general
      register set) and the floating point register set are stored in a
      segment of type PT_NOTE.  We handcraft a couple of extra bfd sections
      that allow standard bfd access to the general registers (.reg) and the
      floating point registers (.reg2).  */

  bfd_cleanup
  elf_core_file_p (bfd *abfd)

Changing _bfd_elf_set_section_contents from:

  pos = hdr->sh_offset + offset;

to:

  pos = section->filepos + offset;

fixes it.  If we do that however, the tail end of
_bfd_elf_set_section_contents ends up as a copy of
_bfd_generic_set_section_contents, so just call the latter, thus
eliminating some duplicate code.

New GDB testcase included, which exercises both patching an executable
and patching a core file.  Patching an executable already works
without this fix, because in that case BFD reads in the sections
table.  Still, we had no testcase for that yet.  In fact, we have no
"set write on" testcases at all, this is the first one.

Tested on x86-64 GNU/Linux, gdb, ld, binutils, and gas.

Change-Id: I0f49f58b48aabab2e269f2959b8fd8a7fe36fdce
---
 bfd/elf.c                        |   9 +--
 gdb/testsuite/gdb.base/patch.c   |  26 ++++++++
 gdb/testsuite/gdb.base/patch.exp | 106 +++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/patch.c
 create mode 100644 gdb/testsuite/gdb.base/patch.exp

diff --git a/bfd/elf.c b/bfd/elf.c
index f046994e3a8..c493aa5b172 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9404,7 +9404,6 @@ _bfd_elf_set_section_contents (bfd *abfd,
 			       bfd_size_type count)
 {
   Elf_Internal_Shdr *hdr;
-  file_ptr pos;
 
   if (! abfd->output_has_begun
       && ! _bfd_elf_compute_section_file_positions (abfd, NULL))
@@ -9457,12 +9456,8 @@ _bfd_elf_set_section_contents (bfd *abfd,
       return true;
     }
 
-  pos = hdr->sh_offset + offset;
-  if (bfd_seek (abfd, pos, SEEK_SET) != 0
-      || bfd_bwrite (location, count, abfd) != count)
-    return false;
-
-  return true;
+  return _bfd_generic_set_section_contents (abfd, section,
+					    location, offset, count);
 }
 
 bool
diff --git a/gdb/testsuite/gdb.base/patch.c b/gdb/testsuite/gdb.base/patch.c
new file mode 100644
index 00000000000..c75f7800c94
--- /dev/null
+++ b/gdb/testsuite/gdb.base/patch.c
@@ -0,0 +1,26 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+/* Value != 0 so it doesn't ends up in .bss, which we wouldn't be able
+   to patch.  */
+int extern_global = 1;
+
+int
+main()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/patch.exp b/gdb/testsuite/gdb.base/patch.exp
new file mode 100644
index 00000000000..96d8f2bcc68
--- /dev/null
+++ b/gdb/testsuite/gdb.base/patch.exp
@@ -0,0 +1,106 @@
+# Copyright 2022 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/>.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Check that we can patch an executable.
+
+with_test_prefix "exec" {
+    clean_restart
+
+    gdb_test_no_output "set write on"
+
+    gdb_load $binfile
+
+    gdb_test "p extern_global" " = 1" "read original value"
+    gdb_test "p extern_global = 2" " = 2" "modify value"
+    gdb_test "p extern_global" " = 2" "value modified"
+
+    clean_restart $binfile
+
+    gdb_test "p extern_global" " = 2" "value modified persisted"
+}
+
+# Check that we can patch a core file.
+
+# Generate a core file.
+with_test_prefix "gcore" {
+    clean_restart $binfile
+
+    if {![runto_main]} {
+	return
+    }
+
+    # Extract the value at PC, and add 1, letting it wrap if
+    # necessary.  This is the value we will poke into memory.
+    set poke_value ""
+    gdb_test_multiple "p/x (*(unsigned char *) \$pc) + 1" "compute poke value" {
+	-re -wrap " = ($hex)" {
+	    set poke_value $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+    if {$poke_value == ""} {
+	return
+    }
+
+    set corefile [standard_output_file gcore.test]
+    set core_supported [gdb_gcore_cmd "$corefile" "save a corefile"]
+    if {!$core_supported} {
+	return
+    }
+}
+
+# Load it into GDB with "set write on", and change the instruction at
+# PC.
+with_test_prefix "load core, write on" {
+    # Don't load a binary, we want to make sure we're patching the
+    # core, not the executable.
+    clean_restart
+
+    gdb_test_no_output "set write on"
+
+    set core_loaded [gdb_core_cmd "$corefile" "re-load generated corefile"]
+    if { $core_loaded == -1 } {
+	# No use proceeding from here.
+	return
+    }
+
+    gdb_test "p/x *(unsigned char *) \$pc = $poke_value" \
+	" = $poke_value" \
+	"poke value"
+    gdb_test "p/x *(unsigned char *) \$pc" \
+	" = $poke_value" \
+	"value modified"
+}
+
+# Re-load it into a new GDB session, now with "set write off", and
+# confirm the value change persisted.
+with_test_prefix "re-load modified core" {
+    # Don't load a binary, we want to make sure we've patched the
+    # core, not the executable.
+    clean_restart
+
+    set core_loaded [gdb_core_cmd "$corefile" "re-load generated corefile"]
+    gdb_assert { $core_loaded != -1 } "core re-loaded"
+
+    gdb_test "p/x *(unsigned char *) \$pc" \
+	" = $poke_value" \
+	"value modified persisted"
+}

base-commit: 845cbaa9ffbfd6a1f7976a6c7f3e4461e4d41993
-- 
2.36.0

Re: [PATCH] Fix "gdb --write" with core files

[Alan Modra]

On Fri, May 13, 2022 at 09:04:29AM +0100, Pedro Alves wrote:
> WDYT of this version, then?

Looks good to me.

-- 
Alan Modra
Australia Development Lab, IBM