[PATCH] libelf: Always update last_offset in updatefile and updatemmap.

[Mark Wielaard <mark at klomp dot org>]

When ELF section data was used, but not updated or marked as dirty and
there also existed non-dirty sections and some padding was needed between
the sections (possibly because of alignment) then elf_update might write
"fill" over some of the existing data. This happened because in that case
the last_position was not updated correctly.

Includes a new testcase fillfile that fails before this patch by showing
fill instead of the expected data in some section data. It succeeds with
this patch.

https://sourceware.org/bugzilla/show_bug.cgi?id=21199

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog          |   6 +
 libelf/elf32_updatefile.c |   9 +-
 tests/ChangeLog           |   7 +
 tests/Makefile.am         |   6 +-
 tests/fillfile.c          | 448 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 469 insertions(+), 7 deletions(-)
 create mode 100644 tests/fillfile.c

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 8539cb5..3da04c0 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,9 @@
+2017-03-27  Mark Wielaard  <mark@klomp.org>
+
+	PR/21199
+	* elf32_updatefile.c (updatemmap): Always update last_positition.
+	(updatefile): Likewise.
+
 2016-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
 	    Mark Wielaard  <mjw@redhat.com>
 
diff --git a/libelf/elf32_updatefile.c b/libelf/elf32_updatefile.c
index 8dd85d1..7ac9951 100644
--- a/libelf/elf32_updatefile.c
+++ b/libelf/elf32_updatefile.c
@@ -343,9 +343,10 @@ __elfw2(LIBELFBITS,updatemmap) (Elf *elf, int change_bo, size_t shnum)
 		  {
 		    fill_mmap (dl->data.d.d_off, last_position, scn_start,
 		               shdr_start, shdr_end);
-		    last_position = scn_start + dl->data.d.d_off;
 		  }
 
+		last_position = scn_start + dl->data.d.d_off;
+
 		if ((scn->flags | dl->flags | elf->flags) & ELF_F_DIRTY)
 		  {
 		    /* Let it go backward if the sections use a bogus
@@ -353,8 +354,6 @@ __elfw2(LIBELFBITS,updatemmap) (Elf *elf, int change_bo, size_t shnum)
 		       user's section data with the latest one, rather than
 		       crashing.  */
 
-		    last_position = scn_start + dl->data.d.d_off;
-
 		    if (unlikely (change_bo))
 		      {
 #if EV_NUM != 2
@@ -728,6 +727,8 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
 		      }
 		  }
 
+		last_offset = scn_start + dl->data.d.d_off;
+
 		if ((scn->flags | dl->flags | elf->flags) & ELF_F_DIRTY)
 		  {
 		    char tmpbuf[MAX_TMPBUF];
@@ -738,8 +739,6 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
 		       user's section data with the latest one, rather than
 		       crashing.  */
 
-		    last_offset = scn_start + dl->data.d.d_off;
-
 		    if (unlikely (change_bo))
 		      {
 #if EV_NUM != 2
diff --git a/tests/ChangeLog b/tests/ChangeLog
index cc6a19b..9b06782 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,10 @@
+2017-03-27  Mark Wielaard  <mark@klomp.org>
+
+	* fillfile.c: New file.
+	* Makefile.am (check_PROGRAMS): Add fillfile.
+	(TESTS): Likewise.
+	(fillfile_LDADD): New variable.
+
 2017-02-15  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* elfstrmerge.c: Include system.h.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index d4659cd..6477b8c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -53,7 +53,8 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  buildid deleted deleted-lib.so aggregate_size vdsosyms \
 		  getsrc_die strptr newdata elfstrtab dwfl-proc-attach \
 		  elfshphehdr elfstrmerge dwelfgnucompressed elfgetchdr \
-		  elfgetzdata elfputzdata zstrptr emptyfile vendorelf
+		  elfgetzdata elfputzdata zstrptr emptyfile vendorelf \
+		  fillfile
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -127,7 +128,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-elfgetzdata.sh run-elfputzdata.sh run-zstrptr.sh \
 	run-compress-test.sh \
 	run-readelf-zdebug.sh run-readelf-zdebug-rel.sh \
-	emptyfile vendorelf
+	emptyfile vendorelf fillfile
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -489,6 +490,7 @@ elfputzdata_LDADD = $(libelf)
 zstrptr_LDADD = $(libelf)
 emptyfile_LDADD = $(libelf)
 vendorelf_LDADD = $(libelf)
+fillfile_LDADD = $(libelf)
 
 # We want to test the libelf header against the system elf.h header.
 # Don't include any -I CPPFLAGS.
diff --git a/tests/fillfile.c b/tests/fillfile.c
new file mode 100644
index 0000000..915e249
--- /dev/null
+++ b/tests/fillfile.c
@@ -0,0 +1,448 @@
+/* Test program for changing data in one section (but not others) with gaps.
+   Copyright (C) 2017 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 <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include ELFUTILS_HEADER(elf)
+#include <gelf.h>
+
+
+/* Index of last string added.  Returned by add_string ().  */
+static size_t stridx = 0;
+
+/* Adds a string and returns the offset in the section.  */
+static size_t
+add_strtab_entry (Elf_Scn *strtab, const char *str)
+{
+  size_t lastidx = stridx;
+  size_t size = strlen (str) + 1;
+
+  Elf_Data *data = elf_newdata (strtab);
+  if (data == NULL)
+    {
+      printf ("cannot create data SHSTRTAB section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  data->d_buf = (char *) str; /* Discards const, but we will not change. */
+  data->d_type = ELF_T_BYTE;
+  data->d_size = size;
+  data->d_align = 1;
+  data->d_version = EV_CURRENT;
+
+  stridx += size;
+  printf ("add_string: '%s', stridx: %zd, lastidx: %zd\n",
+	  str, stridx, lastidx);
+  return lastidx;
+}
+
+static Elf_Scn *
+create_strtab (Elf *elf)
+{
+  // Create strtab section.
+  Elf_Scn *scn = elf_newscn (elf);
+  if (scn == NULL)
+    {
+      printf ("cannot create strings section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Add an empty string to the table as NUL entry for section zero.
+  add_strtab_entry (scn, "");
+
+  GElf_Shdr shdr_mem;
+  GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+  if (shdr == NULL)
+    {
+      printf ("cannot get header for new strtab section: %s\n",
+	      elf_errmsg (-1));
+      exit (1);
+    }
+
+  shdr->sh_type = SHT_STRTAB;
+  shdr->sh_flags = 0;
+  shdr->sh_addr = 0;
+  shdr->sh_link = SHN_UNDEF;
+  shdr->sh_info = SHN_UNDEF;
+  shdr->sh_addralign = 1;
+  shdr->sh_entsize = 0;
+  shdr->sh_name = add_strtab_entry (scn, ".strtab");
+
+  // We have to store the section strtab index in the ELF header.
+  // So sections have actual names.
+  GElf_Ehdr ehdr_mem;
+  GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
+  if (ehdr == NULL)
+    {
+      printf ("cannot get ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  int ndx = elf_ndxscn (scn);
+  ehdr->e_shstrndx = ndx;
+
+  if (gelf_update_ehdr (elf, ehdr) == 0)
+    {
+      printf ("cannot update ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Finished strtab section, update the header.
+  if (gelf_update_shdr (scn, shdr) == 0)
+    {
+      printf ("cannot update STRTAB section header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  return scn;
+}
+
+static char sec_data[] = { 1, 2, 3, 4, 5 };
+static char new_data[] = { 5, 4, 3, 2, 1 };
+
+static void
+add_data_section (Elf *elf, Elf_Scn *strtab, const char *sname)
+{
+  printf ("Add data section %s\n", sname);
+  Elf_Scn *scn = elf_newscn (elf);
+  if (scn == NULL)
+    {
+      printf ("cannot create strings section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  GElf_Shdr shdr_mem;
+  GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+  if (shdr == NULL)
+    {
+      printf ("cannot get header for new %s section: %s\n",
+	      sname, elf_errmsg (-1));
+      exit (1);
+    }
+
+  shdr->sh_type = SHT_PROGBITS;
+  shdr->sh_flags = 0;
+  shdr->sh_addr = 0;
+  shdr->sh_link = SHN_UNDEF;
+  shdr->sh_info = SHN_UNDEF;
+  shdr->sh_addralign = 128;  // Large alignment to force gap between sections.
+  shdr->sh_entsize = 1;
+  shdr->sh_name = add_strtab_entry (strtab, sname);
+
+  if (gelf_update_shdr (scn, shdr) == 0)
+    {
+      printf ("cannot update %s section header: %s\n", sname, elf_errmsg (-1));
+      exit (1);
+    }
+
+  /* Add some data, but less than alignment. */
+  Elf_Data *data = elf_newdata (scn);
+  if (data == NULL)
+    {
+      printf ("cannot update %s section header: %s\n", sname, elf_errmsg (-1));
+      exit (1);
+    }
+  data->d_buf = sec_data;
+  data->d_size = 5;
+}
+
+static void
+check_data (const char *sname, Elf_Data *data, char *buf)
+{
+  printf ("check data %s [", sname);
+  for (int i = 0; i < 5; i++)
+    printf ("%d%s", buf[i], i < 4 ? "," : "");
+  printf ("]\n");
+  if (data == NULL || data->d_buf == NULL)
+    {
+      printf ("No data in section %s\n", sname);
+      exit (1);
+    }
+
+  if (data->d_size != 5 || memcmp (data->d_buf, buf, 5) != 0)
+    {
+      printf ("Wrong data in section %s [", sname);
+      for (size_t i = 0; i < data->d_size; i++)
+	printf ("%d%s", ((char *)data->d_buf)[i],
+		i < data->d_size - 1 ? "," : "");
+      printf ("]\n");
+      exit(1);
+    }
+}
+
+static void
+check_elf (const char *fname, int class, int use_mmap)
+{
+  printf ("\nfname: %s\n", fname);
+  stridx = 0; // Reset strtab strings index
+
+  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s': %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  Elf *elf = elf_begin (fd, use_mmap ? ELF_C_WRITE_MMAP : ELF_C_WRITE, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Create an ELF header.
+  if (gelf_newehdr (elf, class) == 0)
+    {
+      printf ("cannot create ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  GElf_Ehdr ehdr_mem;
+  GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
+  if (ehdr == NULL)
+    {
+      printf ("cannot get ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Initialize header.
+  ehdr->e_ident[EI_DATA] = class == ELFCLASS64 ? ELFDATA2LSB : ELFDATA2MSB;
+  ehdr->e_ident[EI_OSABI] = ELFOSABI_GNU;
+  ehdr->e_type = ET_NONE;
+  ehdr->e_machine = EM_X86_64;
+  ehdr->e_version = EV_CURRENT;
+
+  if (gelf_update_ehdr (elf, ehdr) == 0)
+    {
+      printf ("cannot update ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  Elf_Scn *strtab = create_strtab (elf);
+  add_data_section (elf, strtab, ".data1");
+  add_data_section (elf, strtab, ".data2");
+  add_data_section (elf, strtab, ".data3");
+  add_data_section (elf, strtab, ".data4");
+
+  // Write everything to disk.
+  if (elf_update (elf, ELF_C_WRITE) < 0)
+    {
+      printf ("failure in elf_update(WRITE): %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  /* Reread the ELF from disk now.  */
+  printf ("Rereading %s\n", fname);
+  fd = open (fname, O_RDWR, 0666);
+  if (fd == -1)
+    {
+      printf ("cannot (re)open `%s': %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = elf_begin (fd, use_mmap ? ELF_C_RDWR_MMAP : ELF_C_RDWR, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor read-only: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  /* We are going to change some data (in-place), but want the layout
+     to stay exactly the same. */
+  elf_flagelf (elf, ELF_C_SET, ELF_F_LAYOUT);
+
+  size_t shdrstrndx;
+  if (elf_getshdrstrndx (elf, &shdrstrndx) != 0)
+    {
+      printf ("cannot get shdr str ndx\n");
+      exit (1);
+    }
+  printf ("shdrstrndx: %zd\n", shdrstrndx);
+
+  // Get third data section and change it.
+  Elf_Scn *checkscn = NULL;
+  Elf_Scn *scn = elf_nextscn (elf, NULL);
+  while (scn != NULL)
+    {
+      GElf_Shdr shdr_mem;
+      GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+      if (shdr == NULL)
+	{
+	  printf ("cannot get header for section: %s\n", elf_errmsg (-1));
+	  exit (1);
+	}
+      const char *sname = elf_strptr (elf, shdrstrndx, shdr->sh_name);
+      if (sname != NULL && strcmp (".data3", sname) == 0)
+	checkscn = scn;
+
+      // Get all data, but don't really use it
+      // (this triggered the original bug).
+      Elf_Data *data = elf_getdata (scn, NULL);
+      if (data != NULL && data->d_buf != NULL && data->d_size == 0)
+	{
+	  printf ("Bad data...n");
+	  exit (1);
+	}
+      scn = elf_nextscn (elf, scn);
+    }
+
+  if (checkscn == NULL)
+    {
+      printf ("ELF file doesn't have a .data3 section\n");
+      exit (1);
+    }
+
+  Elf_Data *data = elf_getdata (checkscn, NULL);
+  check_data (".data3", data, sec_data);
+  memcpy (data->d_buf, new_data, 5);
+  elf_flagdata (data, ELF_C_SET, ELF_F_DIRTY);
+
+  // Write everything to disk.
+  if (elf_update (elf, ELF_C_WRITE) < 0)
+    {
+      printf ("failure in elf_update(WRITE): %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // And read it in one last time.
+  printf ("Rereading %s again\n", fname);
+  fd = open (fname, O_RDONLY, 0666);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = elf_begin (fd, use_mmap ? ELF_C_READ_MMAP : ELF_C_READ, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor read-only: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Get all .data sections and check them.
+  Elf_Scn *scn1 = NULL;
+  Elf_Scn *scn2 = NULL;
+  Elf_Scn *scn3 = NULL;
+  Elf_Scn *scn4 = NULL;
+  scn = elf_nextscn (elf, NULL);
+  while (scn != NULL)
+    {
+      GElf_Shdr shdr_mem;
+      GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+      if (shdr == NULL)
+	{
+	  printf ("cannot get header for section: %s\n", elf_errmsg (-1));
+	  exit (1);
+	}
+      const char *sname = elf_strptr (elf, shdrstrndx, shdr->sh_name);
+      if (sname != NULL && strcmp (".data1", sname) == 0)
+	scn1 = scn;
+      else if (sname != NULL && strcmp (".data2", sname) == 0)
+	scn2 = scn;
+      else if (sname != NULL && strcmp (".data3", sname) == 0)
+	scn3 = scn;
+      else if (sname != NULL && strcmp (".data4", sname) == 0)
+	scn4 = scn;
+      scn = elf_nextscn (elf, scn);
+    }
+
+  if (scn1 == NULL)
+    {
+      printf ("ELF file doesn't have a .data1 section\n");
+      exit (1);
+    }
+  data = elf_getdata (scn1, NULL);
+  check_data (".data1", data, sec_data);
+
+  if (scn2 == NULL)
+    {
+      printf ("ELF file doesn't have a .data2 section\n");
+      exit (1);
+    }
+  data = elf_getdata (scn2, NULL);
+  check_data (".data2", data, sec_data);
+
+  if (scn3 == NULL)
+    {
+      printf ("ELF file doesn't have a .data3 section\n");
+      exit (1);
+    }
+  data = elf_getdata (scn3, NULL);
+  check_data (".data3", data, new_data);
+
+  if (scn4 == NULL)
+    {
+      printf ("ELF file doesn't have a .data4 section\n");
+      exit (1);
+    }
+  data = elf_getdata (scn4, NULL);
+  check_data (".data4", data, sec_data);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  unlink (fname);
+}
+
+int
+main (int argc __attribute__ ((unused)),
+      char *argv[] __attribute__ ((unused)))
+{
+  elf_version (EV_CURRENT);
+
+  elf_fill (0xA);
+
+  check_elf ("fill.elf.32", ELFCLASS32, 0);
+  check_elf ("fill.elf.32.mmap", ELFCLASS32, 1);
+  check_elf ("fill.elf.64", ELFCLASS64, 0);
+  check_elf ("fill.elf.64.mmap", ELFCLASS64, 1);
+
+  return 0;
+}
-- 
1.8.3.1