[PATCH] libelf: Mark shdr_flags dirty if offset or size changes during update.

[Mark Wielaard <mark at klomp dot org>]

We forgot to mark the shdr_flags dirty when only the sh_size or
sh_offset changed during elf_update (). This meant that if there were
no other shdr changes we only wrote out the section data, but didn't
write out the shdr table to the file.

Add a testcase that puts some sections in the reverse order and then
writes out the resulting file again without doing any other
updates. This would show the issue after write out of the
(re-reversed) ELF file (the .shstrtab section offset would be wrong
causing all section names to be garbage). Also run a self test on the
ET_REL object files.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog                   |   5 ++
 libelf/elf32_updatenull.c          |   5 +-
 tests/ChangeLog                    |  15 +++++
 tests/Makefile.am                  |   5 +-
 tests/elfcopy.c                    |  98 +++++++++++++++++++++++++---
 tests/elfrdwrnop.c                 | 100 +++++++++++++++++++++++++++++
 tests/run-reverse-sections-self.sh |  45 +++++++++++++
 tests/run-reverse-sections.sh      |  69 ++++++++++++++++++++
 8 files changed, 332 insertions(+), 10 deletions(-)
 create mode 100644 tests/elfrdwrnop.c
 create mode 100755 tests/run-reverse-sections-self.sh
 create mode 100755 tests/run-reverse-sections.sh

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 924ff5917..82e18ebc0 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2019-05-12  Mark Wielaard  <mark@klomp.org>
+
+	* elf32_updatenull.c (updatenull_wrlock): Mark shdr_flags dirty if
+	either offset or size changed.
+
 2019-05-01  Mark Wielaard  <mark@klomp.org>
 
 	* gelf_getnote.c (gelf_getnote): Check n_namesz doesn't overflow
diff --git a/libelf/elf32_updatenull.c b/libelf/elf32_updatenull.c
index 2ce6a5974..303055a02 100644
--- a/libelf/elf32_updatenull.c
+++ b/libelf/elf32_updatenull.c
@@ -366,12 +366,15 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 		    }
 
 		  /* See whether the section size is correct.  */
+		  int size_changed = 0;
 		  update_if_changed (shdr->sh_size, (GElf_Word) offset,
-				     changed);
+				     size_changed);
+		  changed |= size_changed;
 
 		  if (shdr->sh_type != SHT_NOBITS)
 		    size += offset;
 
+		  scn->shdr_flags |= (offset_changed | size_changed);
 		  scn->flags |= changed;
 		}
 
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 49392f1f2..b696f3daf 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,18 @@
+2019-05-12  Mark Wielaard  <mark@klomp.org>
+
+	* Makefile.am (check_PROGRAMS): Add elfrdwrnop.
+	(TESTS): Add run-reverse-sections.sh and
+	run-reverse-sections-self.sh.
+	(EXTRA_DIST): Likewise.
+	(elfrdwrnop): New variable.
+	* elfcopy.c (copy_elf): Add reverse_off argument. Record offsets
+	of sections and swap them when possible.
+	(main): Check for --reverse-off argument. Pass reverse_offs to
+	copy_elf.
+	* run-reverse-sections.sh: New test.
+	* run-reverse-sections-self.sh: Likewise.
+	* elfrdwrnop.c: New file.
+
 2019-04-30  Mark Wielaard  <mark@klomp.org>
 
 	* xlate_notes.c: New file.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 498c1db26..673eccb57 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -60,7 +60,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  fillfile dwarf_default_lower_bound dwarf-die-addr-die \
 		  get-units-invalid get-units-split attr-integrate-skel \
 		  all-dwarf-ranges unit-info next_cfi \
-		  elfcopy addsections xlate_notes
+		  elfcopy addsections xlate_notes elfrdwrnop
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -157,6 +157,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \
 	run-all-dwarf-ranges.sh run-unit-info.sh \
 	run-reloc-bpf.sh \
 	run-next-cfi.sh run-next-cfi-self.sh \
+	run-reverse-sections.sh run-reverse-sections-self.sh \
 	run-copyadd-sections.sh run-copymany-sections.sh \
 	run-typeiter-many.sh run-strip-test-many.sh \
 	run-strip-version.sh run-xlate-note.sh
@@ -418,6 +419,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     run-unit-info.sh run-next-cfi.sh run-next-cfi-self.sh \
 	     testfile-riscv64.bz2 testfile-riscv64-s.bz2 \
 	     testfile-riscv64-core.bz2 \
+	     run-reverse-sections.sh run-reverse-sections-self.sh \
 	     run-copyadd-sections.sh run-copymany-sections.sh \
 	     run-typeiter-many.sh run-strip-test-many.sh \
 	     testfile-debug-rel-ppc64-g.o.bz2 \
@@ -595,6 +597,7 @@ next_cfi_LDADD = $(libelf) $(libdw)
 elfcopy_LDADD = $(libelf)
 addsections_LDADD = $(libelf)
 xlate_notes_LDADD = $(libelf)
+elfrdwrnop_LDADD = $(libelf)
 
 # We want to test the libelf header against the system elf.h header.
 # Don't include any -I CPPFLAGS. Except when we install our own elf.h.
diff --git a/tests/elfcopy.c b/tests/elfcopy.c
index 9000cc969..d457badbc 100644
--- a/tests/elfcopy.c
+++ b/tests/elfcopy.c
@@ -69,9 +69,11 @@ setshstrndx (Elf *elf, size_t ndx)
 
 /* Copies all elements of an ELF file either using mmap or read.  */
 static void
-copy_elf (const char *in, const char *out, bool use_mmap)
+copy_elf (const char *in, const char *out, bool use_mmap, bool reverse_offs)
 {
-  printf ("\ncopy_elf: %s -> %s (%s)\n", in, out, use_mmap ? "mmap" : "read");
+  printf ("\ncopy_elf: %s -> %s (%s,%s)\n", in, out,
+	  use_mmap ? "mmap" : "read",
+	  reverse_offs ? "reverse" : "same");
 
   /* Existing ELF file.  */
   int fda = open (in, O_RDONLY);
@@ -182,8 +184,28 @@ copy_elf (const char *in, const char *out, bool use_mmap)
 	}
     }
 
+  GElf_Off *offs = NULL;
+  size_t shnum;
+  if (reverse_offs)
+    {
+      if (elf_getshdrnum (elfa, &shnum) < 0)
+	{
+	  printf ("couldn't get shdrnum: %s\n", elf_errmsg (-1));
+	  exit (1);
+	}
+
+      offs = (GElf_Off *) malloc (shnum * sizeof (GElf_Off));
+      if (offs == NULL)
+	{
+	  printf ("couldn't allocate memory for offs\n");
+	  exit (1);
+	}
+    }
+
   /* Copy all sections, headers and data.  */
   Elf_Scn *scn = NULL;
+  size_t last_off = 0;
+  GElf_Shdr last_shdr = { .sh_type = SHT_NULL };
   while ((scn = elf_nextscn (elfa, scn)) != NULL)
     {
       /* Get the header.  */
@@ -194,6 +216,34 @@ copy_elf (const char *in, const char *out, bool use_mmap)
 	  exit (1);
 	}
 
+      if (reverse_offs)
+	{
+	  offs[last_off] = shdr.sh_offset;
+
+	  if (last_shdr.sh_type != SHT_NULL
+	      && last_shdr.sh_addralign == shdr.sh_addralign
+	      && shdr.sh_addralign == 1
+	      && last_shdr.sh_type != SHT_NOBITS
+	      && shdr.sh_type != SHT_NOBITS
+	      && (phnum == 0
+		  || ((shdr.sh_flags & SHF_ALLOC) == 0
+		      && (last_shdr.sh_flags & SHF_ALLOC) == 0)))
+	    {
+	      printf ("Swapping offsets of section %zd and %zd\n",
+		      last_off, last_off + 1);
+	      GElf_Word off = offs[last_off - 1];
+	      offs[last_off - 1] = off + shdr.sh_size;
+	      offs[last_off] = off;
+	      last_shdr.sh_type = SHT_NULL;
+	    }
+	  else
+	    {
+	      last_shdr = shdr;
+	      offs[last_off] = shdr.sh_offset;
+	    }
+	  last_off++;
+	}
+
       /* Create new section.  */
       Elf_Scn *new_scn = elf_newscn (elfb);
       if (new_scn == NULL)
@@ -223,9 +273,34 @@ copy_elf (const char *in, const char *out, bool use_mmap)
 	}
     }
 
-  /* Write everything to disk.  If there are any phdrs then we want
-     the exact same layout.  Do we want ELF_F_PERMISSIVE?  */
-  if (phnum > 0)
+  if (reverse_offs)
+    {
+      last_off = 0;
+      scn = NULL;
+      while ((scn = elf_nextscn (elfb, scn)) != NULL)
+	{
+	  GElf_Shdr shdr;
+	  if (gelf_getshdr (scn, &shdr) == NULL)
+	    {
+	      printf ("couldn't get shdr for updating: %s\n", elf_errmsg (-1));
+	      exit (1);
+	    }
+
+	  shdr.sh_offset = offs[last_off++];
+
+	  if (gelf_update_shdr (scn, &shdr) == 0)
+	    {
+	      printf ("couldn't update shdr sh_off: %s\n", elf_errmsg (-1));
+	      exit (1);
+	    }
+	}
+      free (offs);
+    }
+
+  /* Write everything to disk.  If there are any phdrs, or we want to
+     update the offsets, then we want the exact same layout.  Do we
+     want ELF_F_PERMISSIVE?  */
+  if (phnum > 0 || reverse_offs)
     elf_flagelf (elfb, ELF_C_SET, ELF_F_LAYOUT);
   if (elf_update (elfb, ELF_C_WRITE) < 0)
     {
@@ -264,9 +339,9 @@ main (int argc, const char *argv[])
   elf_version (EV_CURRENT);
 
   /* Takes the given file, and create a new identical one.  */
-  if (argc < 3 || argc > 4)
+  if (argc < 3 || argc > 5)
     {
-      fprintf (stderr, "elfcopy [--mmap] in.elf out.elf\n");
+      fprintf (stderr, "elfcopy [--mmap] [--reverse-offs] in.elf out.elf\n");
       exit (1);
     }
 
@@ -278,9 +353,16 @@ main (int argc, const char *argv[])
       argn++;
     }
 
+  bool reverse_offs = false;
+  if (strcmp (argv[argn], "--reverse-offs") == 0)
+    {
+      reverse_offs = true;
+      argn++;
+    }
+
   const char *in = argv[argn++];
   const char *out = argv[argn];
-  copy_elf (in, out, use_mmap);
+  copy_elf (in, out, use_mmap, reverse_offs);
 
   return 0;
 }
diff --git a/tests/elfrdwrnop.c b/tests/elfrdwrnop.c
new file mode 100644
index 000000000..997150b95
--- /dev/null
+++ b/tests/elfrdwrnop.c
@@ -0,0 +1,100 @@
+/* Test program for reading and writing out the same file in-place
+   Copyright (C) 2019 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file 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.
+
+   elfutils 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/>.  */
+
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include ELFUTILS_HEADER(elf)
+#include <gelf.h>
+
+
+int
+main (int argc, const char *argv[])
+{
+  /* Takes the given file, and create a new identical one.  */
+  if (argc != 2)
+    {
+      fprintf (stderr, "elfrdwrnop elf-file\n");
+      exit (1);
+    }
+
+  elf_version (EV_CURRENT);
+
+  const char *name = argv[1];
+  printf ("elfrdwrdnop %s\n", name);
+
+  int fd = open (name, O_RDWR);
+  if (fd < 0)
+    {
+      fprintf (stderr, "Couldn't open file '%s': %s\n",
+	       name, strerror (errno));
+      exit (1);
+    }
+
+  Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL);
+  if (elf == NULL)
+    {
+      fprintf (stderr, "Couldn't open ELF file '%s': %s\n",
+	       name, elf_errmsg (-1));
+      exit (1);
+    }
+
+  /* Write everything to disk.  If there are any phdrs, then we want
+     the exact same layout.  */
+  size_t phnum;
+  if (elf_getphdrnum (elf, &phnum) != 0)
+    {
+      printf ("cannot get phdrs: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (phnum > 0)
+    elf_flagelf (elf, ELF_C_SET, ELF_F_LAYOUT);
+
+  if (elf_update (elf, ELF_C_WRITE) < 0)
+    {
+      printf ("failure in elf_update: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("couldn't cleanup elf '%s': %s\n", name, elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (close (fd) != 0)
+    {
+      printf ("couldn't close '%s': %s\n", name, strerror (errno));
+      exit (1);
+    }
+
+  return 0;
+}
diff --git a/tests/run-reverse-sections-self.sh b/tests/run-reverse-sections-self.sh
new file mode 100755
index 000000000..6cece4188
--- /dev/null
+++ b/tests/run-reverse-sections-self.sh
@@ -0,0 +1,45 @@
+#! /bin/sh
+# Copyright (C) 2019 Red Hat, Inc.
+# This file is part of elfutils.
+#
+# This file 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.
+#
+# elfutils 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/>.
+
+. $srcdir/test-subr.sh
+
+test_reverse_self ()
+{
+  in_file="$1"
+  base_name="$(basename ${in_file})"
+  out_file="${base_name}.rev"
+  out_file_mmap="${out_file}.mmap"
+
+  tempfiles ${out_file} ${out_file_mmap}
+
+  # Reverse the offsets (the files should still be the same otherwise)
+  testrun ${abs_builddir}/elfcopy --reverse-offs ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elfcmp ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elflint --gnu ${out_file}
+  # An in-place nop will likely revert them back
+  testrun ${abs_builddir}/elfrdwrnop ${out_file}
+  testrun ${abs_top_builddir}/src/elfcmp ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elflint --gnu ${out_file}
+}
+
+# Only really makes sense for ET_REL files, otherwise we need to
+# keep order for the allocated sections.
+for file in $self_test_files_obj; do
+  test_reverse_self $file
+done
+
+exit 0
diff --git a/tests/run-reverse-sections.sh b/tests/run-reverse-sections.sh
new file mode 100755
index 000000000..102a12614
--- /dev/null
+++ b/tests/run-reverse-sections.sh
@@ -0,0 +1,69 @@
+#! /bin/sh
+# Copyright (C) 2019 Red Hat, Inc.
+# This file is part of elfutils.
+#
+# This file 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.
+#
+# elfutils 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/>.
+
+. $srcdir/test-subr.sh
+
+test_reverse ()
+{
+  in_file="$1"
+  out_file="${in_file}.rev"
+  out_file_mmap="${out_file}.mmap"
+
+  testfiles ${in_file}
+  tempfiles ${out_file} ${out_file_mmap}
+
+  # Reverse the offsets (the files should still be the same otherwise)
+  testrun ${abs_builddir}/elfcopy --reverse-offs ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elfcmp ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elflint --gnu ${out_file}
+  # An in-place nop will likely revert them back
+  testrun ${abs_builddir}/elfrdwrnop ${out_file}
+  testrun ${abs_top_builddir}/src/elfcmp ${in_file} ${out_file}
+  testrun ${abs_top_builddir}/src/elflint --gnu ${out_file}
+}
+
+# A collection of random testfiles to test 32/64bit, little/big endian
+# and non-ET_REL (with phdrs)/ET_REL (without phdrs).
+
+# 32bit, big endian, rel
+test_reverse testfile29
+
+# 64bit, big endian, rel
+test_reverse testfile23
+
+# 32bit, little endian, rel
+test_reverse testfile9
+
+# 64bit, little endian, rel
+test_reverse testfile38
+
+# 32bit, big endian, non-rel
+test_reverse testfile26
+
+# 64bit, big endian, non-rel
+test_reverse testfile27
+
+# 32bit, little endian, non-rel
+test_reverse testfile
+
+# 64bit, little endian, non-rel
+# Don't use testfile10. It has section headers in the middle of the file.
+# Same for testfile12. It is legal, but not the point of this testcase.
+# test_reverse testfile10
+test_reverse testfile13
+
+exit 0
-- 
2.20.1