[PATCH] libelf: Fix elf_newdata when raw data has been read, but not converted.

[PATCH] libelf: Fix elf_newdata when raw data has been read, but not converted.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 13472 bytes --]

When ELF data for a section has been read by elf_rawdata, data_read
and rawdata_base are set, but data_list_rear will not be set until the
data will be converted (by elf_getdata). elf_newdata would overwrite
the existing data in that case.

Add newdata test that calls elf_newdata before and after elf_rawdata
and elf_getdata and checks the new size of the section.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog     |   4 +
 libelf/elf_newdata.c |  13 +-
 tests/ChangeLog      |   7 +
 tests/Makefile.am    |   5 +-
 tests/newdata.c      | 365 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 390 insertions(+), 4 deletions(-)
 create mode 100644 tests/newdata.c

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 6699052..36a6031 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,5 +1,9 @@
 2015-01-20  Mark Wielaard  <mjw@redhat.com>
 
+	* elf_newdata.c (elf_newdata): Check scn->rawdata_base.
+
+2015-01-20  Mark Wielaard  <mjw@redhat.com>
+
 	* elf_strptr.c (elf_strptr): Call __elf[32|64]_getshdr_rdlock if
 	necessary.
 
diff --git a/libelf/elf_newdata.c b/libelf/elf_newdata.c
index 90d1813..06baeb5 100644
--- a/libelf/elf_newdata.c
+++ b/libelf/elf_newdata.c
@@ -1,5 +1,5 @@
 /* Create new, empty section data.
-   Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
+   Copyright (C) 1998, 1999, 2000, 2001, 2002, 2015 Red Hat, Inc.
    This file is part of elfutils.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 1998.
 
@@ -64,7 +64,16 @@ elf_newdata (Elf_Scn *scn)
 
   rwlock_wrlock (scn->elf->lock);
 
-  if (scn->data_read && scn->data_list_rear == NULL)
+  /* data_read is set when data has been read from the ELF image or
+     when a new section has been created by elf_newscn.  If data has
+     been read from the ELF image, then rawdata_base will point to raw
+     data.  If data_read has been set by elf_newscn, then rawdata_base
+     will be NULL.  data_list_rear will be set by elf_getdata if the
+     data has been converted, or by this function, elf_newdata, when
+     new data has been added.  */
+  if (scn->data_read
+      && scn->rawdata_base == NULL
+      && scn->data_list_rear == NULL)
     {
       /* This means the section was created by the user and this is the
 	 first data.  */
diff --git a/tests/ChangeLog b/tests/ChangeLog
index f94d9be..d41d0e1 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,5 +1,12 @@
 2015-01-20  Mark Wielaard  <mjw@redhat.com>
 
+	* Makefile.am (check_PROGRAMS): Add newdata.
+	(TESTS): Likewise.
+	(newdata_LDADD): new variable.
+	* newdata.c: New test.
+
+2015-01-20  Mark Wielaard  <mjw@redhat.com>
+
 	* strptr.c: New file.
 	* run-strptr.sh: New test.
 	* Makefile.am (check_PROGRAMS): Add strptr.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c3364a2..4420e8b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -51,7 +51,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  dwfl-report-elf-align varlocs backtrace backtrace-child \
 		  backtrace-data backtrace-dwarf debuglink debugaltlink \
 		  buildid deleted deleted-lib.so aggregate_size vdsosyms \
-		  getsrc_die strptr
+		  getsrc_die strptr newdata
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -113,7 +113,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-backtrace-demangle.sh run-stack-d-test.sh run-stack-i-test.sh \
 	run-readelf-dwz-multi.sh run-allfcts-multi.sh run-deleted.sh \
 	run-linkmap-cut.sh run-aggregate-size.sh vdsosyms run-readelf-A.sh \
-	run-getsrc-die.sh run-strptr.sh
+	run-getsrc-die.sh run-strptr.sh newdata
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -426,6 +426,7 @@ aggregate_size_LDADD = $(libdw) $(libelf)
 vdsosyms_LDADD = $(libdw) $(libelf)
 getsrc_die_LDADD = $(libdw) $(libelf)
 strptr_LDADD = $(libelf)
+newdata_LDADD = $(libelf)
 
 if GCOV
 check: check-am coverage
diff --git a/tests/newdata.c b/tests/newdata.c
new file mode 100644
index 0000000..7dfe962
--- /dev/null
+++ b/tests/newdata.c
@@ -0,0 +1,365 @@
+/* Test program for elf_newdata function.
+   Copyright (C) 2015 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>
+
+// Random data string (16 bytes).
+static char *DATA = "123456789ABCDEF";
+static size_t DATA_LEN = 16;
+
+static void
+add_section_data (Elf *elf, char *buf, size_t len)
+{
+  printf ("Adding %zd bytes.\n", len);
+
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  Elf_Data *data = elf_newdata (scn);
+  if (data == NULL)
+    {
+      printf ("cannot create newdata for section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  data->d_buf = buf;
+  data->d_type = ELF_T_BYTE;
+  data->d_size = len;
+  data->d_align = 1;
+  data->d_version = EV_CURRENT;
+
+  // Let the library compute the internal structure information.
+  if (elf_update (elf, ELF_C_NULL) < 0)
+    {
+      printf ("failure in elf_update(NULL): %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+}
+
+static Elf *
+create_elf (int fd, int class)
+{
+  Elf *elf = elf_begin (fd, 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] = ELFCLASS32 ? ELFDATA2LSB : ELFDATA2MSB;
+  ehdr->e_ident[EI_OSABI] = ELFOSABI_GNU;
+  ehdr->e_type = ET_NONE;
+  ehdr->e_machine = class == ELFCLASS32 ? EM_PPC : EM_X86_64;
+  ehdr->e_version = EV_CURRENT;
+
+  // Update the ELF header.
+  if (gelf_update_ehdr (elf, ehdr) == 0)
+    {
+      printf ("cannot update ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Create a section.
+  Elf_Scn *scn = elf_newscn (elf);
+  if (scn == NULL)
+    {
+      printf ("cannot create new 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 data section: %s\n", 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 = 1;
+  shdr->sh_entsize = 1;
+  shdr->sh_name = 0;
+
+  // Finish section, update the header.
+  if (gelf_update_shdr (scn, shdr) == 0)
+    {
+      printf ("cannot update header for DATA section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Add some data to the section.
+  add_section_data (elf, DATA, DATA_LEN);
+
+  // 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);
+    }
+
+  return elf;
+}
+
+static Elf *
+read_elf (int fd)
+{
+  printf ("Reading ELF file\n");
+  Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor read-again: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  return elf;
+}
+
+static void
+check_section_size (Elf *elf, size_t size)
+{
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data 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 DATA section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (shdr->sh_size == size)
+    printf ("OK %zd bytes.\n", size);
+  else
+    {
+      printf ("BAD size, expected %zd, got %" PRIu64 "\n",
+	      size, shdr->sh_size);
+      exit (-1);
+    }
+}
+
+static void
+check_elf (int class)
+{
+  static const char *fname;
+  fname = class == ELFCLASS32 ? "newdata.elf32" : "newdata.elf64";
+
+  printf ("check_elf: %s\n", fname);
+
+  int fd = open (fname, O_RDWR|O_CREAT|O_TRUNC, 00666);
+  if (fd == -1)
+    {
+      printf ("cannot create `%s': %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  Elf *elf = create_elf (fd, class);
+  check_section_size (elf, DATA_LEN);
+
+  // Add some more data (won't be written to disk).
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data directly.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+
+  // Add some more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data after raw reading.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+
+  // Get raw data before adding new data.
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  printf ("elf_rawdata\n");
+  Elf_Data *data = elf_rawdata (scn, NULL);
+  if (data == NULL)
+    {
+      printf ("couldn't get raw data from section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (data->d_size != DATA_LEN)
+    {
+      printf ("Unexpected Elf_Data: %zd", data->d_size);
+      exit (1);
+    }
+
+  // Now add more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data after data reading.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+
+  // Get raw data before adding new data.
+  scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  printf ("elf_getdata\n");
+  data = elf_getdata (scn, NULL);
+  if (data == NULL)
+    {
+      printf ("couldn't get raw data from section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (data->d_size != DATA_LEN)
+    {
+      printf ("Unexpected Elf_Data: %zd", data->d_size);
+      exit (1);
+    }
+
+  // Now add more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+
+  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)))
+{
+  // Initialize libelf.
+  elf_version (EV_CURRENT);
+
+  check_elf (ELFCLASS32);
+  check_elf (ELFCLASS64);
+
+  return 0;
+}
-- 
1.8.3.1

Re: [PATCH] libelf: Fix elf_newdata when raw data has been read, but not converted.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Wed, Jan 21, 2015 at 05:57:03PM +0100, Mark Wielaard wrote:
> Subject: [PATCH] libelf: Fix elf_newdata when raw ELF file/image data is
>  available.
> 
> When ELF data for a section has been read by elf_rawdata, data_read
> and rawdata_base are set, but data_list_rear will not be set until the
> data will be converted (by elf_getdata). elf_newdata would overwrite
> the existing data in that case. Both elf_getdata and elf_update rely
> on the fact that when data_list_rear is set they don't have to look
> at the raw data anymore. So make sure we update the data list properly
> before adding any new data and raw data is available in elf_newdata.
> 
> Add newdata test that calls elf_newdata before and after elf_rawdata
> and elf_getdata and checks the new size and contents of the section.

I pushed this to master.

Re: [PATCH] libelf: Fix elf_newdata when raw data has been read, but not converted.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On Tue, Jan 20, 2015 at 10:31:55PM +0100, Mark Wielaard wrote:
> When ELF data for a section has been read by elf_rawdata, data_read
> and rawdata_base are set, but data_list_rear will not be set until the
> data will be converted (by elf_getdata). elf_newdata would overwrite
> the existing data in that case.

This was still not completely correct. Unlike what the comment in the
elf_newdata code (and the description of struct Elf_Scn in libelfP.h)
imply both elf_getdata and elf_update rely on the fact that when
data_list_rear is set they don't have to read/convert and data from
the ELF image anymore. That means we do have to make sure that is
done when adding new data to the section with elf_newdata. Otherwise
elf_getdata might return the wrong data contents and elf_update might
write garbage (or randomly write fill characters).

I updated the patch to more proactively read the existing data when
needed in elf_newdata and updated the testcase to also check the contents
of the section data (and not just the size).

I have another test for this (for related bug in elf_strptr). Once those
tests are in we could look at making the code more lazy in this case by
changing elf_getdata and elf_update to take unread/converted raw data
into account.


[-- Attachment #2: 0001-libelf-Fix-elf_newdata-when-raw-ELF-file-image-data-.patch --]
[-- Type: text/plain, Size: 20205 bytes --]

>From 1be76fe8259d0a7dacf1b357a8b18738234bab1b Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Tue, 20 Jan 2015 21:55:55 +0100
Subject: [PATCH] libelf: Fix elf_newdata when raw ELF file/image data is
 available.

When ELF data for a section has been read by elf_rawdata, data_read
and rawdata_base are set, but data_list_rear will not be set until the
data will be converted (by elf_getdata). elf_newdata would overwrite
the existing data in that case. Both elf_getdata and elf_update rely
on the fact that when data_list_rear is set they don't have to look
at the raw data anymore. So make sure we update the data list properly
before adding any new data and raw data is available in elf_newdata.

Add newdata test that calls elf_newdata before and after elf_rawdata
and elf_getdata and checks the new size and contents of the section.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog     |   9 ++
 libelf/elf_getdata.c |  76 +++++-----
 libelf/elf_newdata.c |  39 ++++-
 libelf/libelfP.h     |   6 +
 tests/ChangeLog      |   7 +
 tests/Makefile.am    |   5 +-
 tests/newdata.c      | 397 +++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 496 insertions(+), 43 deletions(-)
 create mode 100644 tests/newdata.c

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 6699052..c43a7fe 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,5 +1,14 @@
 2015-01-20  Mark Wielaard  <mjw@redhat.com>
 
+	* libelfP.h (__elf_strptr_internal): New function declaration.
+	* elf_getdata.c (__libelf_set_data_list_rdlock): New internal
+	function extracted from...
+	(__elf_getdata_rdlock): ... here.
+	* elf_newdata.c (elf_newdata): Check scn->rawdata_base and update
+	datalist if necessary.
+
+2015-01-20  Mark Wielaard  <mjw@redhat.com>
+
 	* elf_strptr.c (elf_strptr): Call __elf[32|64]_getshdr_rdlock if
 	necessary.
 
diff --git a/libelf/elf_getdata.c b/libelf/elf_getdata.c
index 0aeb997..e8f022f 100644
--- a/libelf/elf_getdata.c
+++ b/libelf/elf_getdata.c
@@ -337,6 +337,44 @@ __libelf_set_rawdata (Elf_Scn *scn)
   return result;
 }
 
+void
+internal_function
+__libelf_set_data_list_rdlock (Elf_Scn *scn, int wrlocked)
+{
+  if (scn->rawdata.d.d_buf != NULL && scn->rawdata.d.d_size > 0)
+    {
+      Elf *elf = scn->elf;
+
+      /* Upgrade the lock to a write lock if necessary and check
+	 nobody else already did the work.  */
+      if (!wrlocked)
+	{
+	  rwlock_unlock (elf->lock);
+	  rwlock_wrlock (elf->lock);
+	  if (scn->data_list_rear != NULL)
+	    return;
+	}
+
+      /* Convert according to the version and the type.   */
+      convert_data (scn, __libelf_version, elf->class,
+		    (elf->class == ELFCLASS32
+		     || (offsetof (struct Elf, state.elf32.ehdr)
+			 == offsetof (struct Elf, state.elf64.ehdr))
+		     ? elf->state.elf32.ehdr->e_ident[EI_DATA]
+		     : elf->state.elf64.ehdr->e_ident[EI_DATA]),
+		    scn->rawdata.d.d_size, scn->rawdata.d.d_type);
+    }
+  else
+    {
+      /* This is an empty or NOBITS section.  There is no buffer but
+	 the size information etc is important.  */
+      scn->data_list.data.d = scn->rawdata.d;
+      scn->data_list.data.s = scn;
+    }
+
+  scn->data_list_rear = &scn->data_list;
+}
+
 Elf_Data *
 internal_function
 __elf_getdata_rdlock (scn, data)
@@ -427,42 +465,10 @@ __elf_getdata_rdlock (scn, data)
      empty in case the section has size zero (for whatever reason).
      Now create the converted data in case this is necessary.  */
   if (scn->data_list_rear == NULL)
-    {
-      if (scn->rawdata.d.d_buf != NULL && scn->rawdata.d.d_size > 0)
-	{
-	  if (!locked)
-	    {
-	      rwlock_unlock (elf->lock);
-	      rwlock_wrlock (elf->lock);
-	      if (scn->data_list_rear != NULL)
-		goto pass;
-	    }
-
-	  /* Convert according to the version and the type.   */
-	  convert_data (scn, __libelf_version, elf->class,
-			(elf->class == ELFCLASS32
-			 || (offsetof (struct Elf, state.elf32.ehdr)
-			     == offsetof (struct Elf, state.elf64.ehdr))
-			 ? elf->state.elf32.ehdr->e_ident[EI_DATA]
-			 : elf->state.elf64.ehdr->e_ident[EI_DATA]),
-			scn->rawdata.d.d_size, scn->rawdata.d.d_type);
-	}
-      else
-	{
-	  /* This is an empty or NOBITS section.  There is no buffer but
-	     the size information etc is important.  */
-	  scn->data_list.data.d = scn->rawdata.d;
-	  scn->data_list.data.s = scn;
-	}
-
-      scn->data_list_rear = &scn->data_list;
-    }
+    __libelf_set_data_list_rdlock (scn, locked);
 
-  /* If no data is present we cannot return any.  */
-  if (scn->data_list_rear != NULL)
-  pass:
-    /* Return the first data element in the list.  */
-    result = &scn->data_list.data.d;
+  /* Return the first data element in the list.  */
+  result = &scn->data_list.data.d;
 
  out:
   return result;
diff --git a/libelf/elf_newdata.c b/libelf/elf_newdata.c
index 90d1813..f6609a8 100644
--- a/libelf/elf_newdata.c
+++ b/libelf/elf_newdata.c
@@ -1,5 +1,5 @@
 /* Create new, empty section data.
-   Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
+   Copyright (C) 1998, 1999, 2000, 2001, 2002, 2015 Red Hat, Inc.
    This file is part of elfutils.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 1998.
 
@@ -64,6 +64,25 @@ elf_newdata (Elf_Scn *scn)
 
   rwlock_wrlock (scn->elf->lock);
 
+  /* data_read is set when data has been read from the ELF image or
+     when a new section has been created by elf_newscn.  If data has
+     been read from the ELF image, then rawdata_base will point to raw
+     data.  If data_read has been set by elf_newscn, then rawdata_base
+     will be NULL.  data_list_rear will be set by elf_getdata if the
+     data has been converted, or by this function, elf_newdata, when
+     new data has been added.
+
+     Currently elf_getdata and elf_update rely on the fact that when
+     data_list_read is not NULL all they have to do is walk the data
+     list. They will ignore any (unread) raw data in that case.
+
+     So we need to make sure the data list is setup if there is
+     already data available.  */
+  if (scn->data_read
+      && scn->rawdata_base != NULL
+      && scn->data_list_rear == NULL)
+    __libelf_set_data_list_rdlock (scn, 1);
+
   if (scn->data_read && scn->data_list_rear == NULL)
     {
       /* This means the section was created by the user and this is the
@@ -73,6 +92,19 @@ elf_newdata (Elf_Scn *scn)
     }
   else
     {
+      /* It would be more efficient to create new data without
+	 reading/converting the data from the file.  But then we
+	 have to remember this.  Currently elf_getdata and
+	 elf_update rely on the fact that they don't have to
+	 load/convert any data if data_list_rear is set.  */
+      if (scn->data_read == 0)
+	{
+	  if (__libelf_set_rawdata_wrlock (scn) != 0)
+	    /* Something went wrong.  The error value is already set.  */
+	    goto out;
+	  __libelf_set_data_list_rdlock (scn, 1);
+	}
+
       /* Create a new, empty data descriptor.  */
       result = (Elf_Data_List *) calloc (1, sizeof (Elf_Data_List));
       if (result == NULL)
@@ -82,11 +114,6 @@ elf_newdata (Elf_Scn *scn)
 	}
 
       result->flags = ELF_F_DIRTY | ELF_F_MALLOCED;
-
-      if (scn->data_list_rear == NULL)
-	/* We create new data without reading/converting the data from the
-	   file.  That is fine but we have to remember this.  */
-	scn->data_list_rear = &scn->data_list;
     }
 
   /* Set the predefined values.  */
diff --git a/libelf/libelfP.h b/libelf/libelfP.h
index 3b24e75..0ad4071 100644
--- a/libelf/libelfP.h
+++ b/libelf/libelfP.h
@@ -532,6 +532,12 @@ extern Elf_Data *__elf_getdata_rdlock (Elf_Scn *__scn, Elf_Data *__data)
      internal_function;
 extern Elf_Data *__elf_rawdata_internal (Elf_Scn *__scn, Elf_Data *__data)
      attribute_hidden;
+/* Should be called to setup first section data element if
+   data_list_rear is NULL and we know data_read is set and there is
+   raw data available.  Might upgrade the ELF lock from a read to a
+   write lock.  If the lock is already a write lock set wrlocked.  */
+extern void __libelf_set_data_list_rdlock (Elf_Scn *scn, int wrlocked)
+  attribute_hidden;
 extern char *__elf_strptr_internal (Elf *__elf, size_t __index,
 				    size_t __offset) attribute_hidden;
 extern Elf_Data *__elf32_xlatetom_internal (Elf_Data *__dest,
diff --git a/tests/ChangeLog b/tests/ChangeLog
index f94d9be..d41d0e1 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,5 +1,12 @@
 2015-01-20  Mark Wielaard  <mjw@redhat.com>
 
+	* Makefile.am (check_PROGRAMS): Add newdata.
+	(TESTS): Likewise.
+	(newdata_LDADD): new variable.
+	* newdata.c: New test.
+
+2015-01-20  Mark Wielaard  <mjw@redhat.com>
+
 	* strptr.c: New file.
 	* run-strptr.sh: New test.
 	* Makefile.am (check_PROGRAMS): Add strptr.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c3364a2..4420e8b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -51,7 +51,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  dwfl-report-elf-align varlocs backtrace backtrace-child \
 		  backtrace-data backtrace-dwarf debuglink debugaltlink \
 		  buildid deleted deleted-lib.so aggregate_size vdsosyms \
-		  getsrc_die strptr
+		  getsrc_die strptr newdata
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -113,7 +113,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-backtrace-demangle.sh run-stack-d-test.sh run-stack-i-test.sh \
 	run-readelf-dwz-multi.sh run-allfcts-multi.sh run-deleted.sh \
 	run-linkmap-cut.sh run-aggregate-size.sh vdsosyms run-readelf-A.sh \
-	run-getsrc-die.sh run-strptr.sh
+	run-getsrc-die.sh run-strptr.sh newdata
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -426,6 +426,7 @@ aggregate_size_LDADD = $(libdw) $(libelf)
 vdsosyms_LDADD = $(libdw) $(libelf)
 getsrc_die_LDADD = $(libdw) $(libelf)
 strptr_LDADD = $(libelf)
+newdata_LDADD = $(libelf)
 
 if GCOV
 check: check-am coverage
diff --git a/tests/newdata.c b/tests/newdata.c
new file mode 100644
index 0000000..99952f2
--- /dev/null
+++ b/tests/newdata.c
@@ -0,0 +1,397 @@
+/* Test program for elf_newdata function.
+   Copyright (C) 2015 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>
+
+// Random data string (16 bytes).
+static char *DATA = "123456789ABCDEF";
+static size_t DATA_LEN = 16;
+
+static void
+add_section_data (Elf *elf, char *buf, size_t len)
+{
+  printf ("Adding %zd bytes.\n", len);
+
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  Elf_Data *data = elf_newdata (scn);
+  if (data == NULL)
+    {
+      printf ("cannot create newdata for section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  data->d_buf = buf;
+  data->d_type = ELF_T_BYTE;
+  data->d_size = len;
+  data->d_align = 1;
+  data->d_version = EV_CURRENT;
+
+  // Let the library compute the internal structure information.
+  if (elf_update (elf, ELF_C_NULL) < 0)
+    {
+      printf ("failure in elf_update(NULL): %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+}
+
+static Elf *
+create_elf (int fd, int class)
+{
+  Elf *elf = elf_begin (fd, 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] = ELFCLASS32 ? ELFDATA2LSB : ELFDATA2MSB;
+  ehdr->e_ident[EI_OSABI] = ELFOSABI_GNU;
+  ehdr->e_type = ET_NONE;
+  ehdr->e_machine = class == ELFCLASS32 ? EM_PPC : EM_X86_64;
+  ehdr->e_version = EV_CURRENT;
+
+  // Update the ELF header.
+  if (gelf_update_ehdr (elf, ehdr) == 0)
+    {
+      printf ("cannot update ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Create a section.
+  Elf_Scn *scn = elf_newscn (elf);
+  if (scn == NULL)
+    {
+      printf ("cannot create new 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 data section: %s\n", 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 = 1;
+  shdr->sh_entsize = 1;
+  shdr->sh_name = 0;
+
+  // Finish section, update the header.
+  if (gelf_update_shdr (scn, shdr) == 0)
+    {
+      printf ("cannot update header for DATA section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Add some data to the section.
+  add_section_data (elf, DATA, DATA_LEN);
+
+  // 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);
+    }
+
+  return elf;
+}
+
+static Elf *
+read_elf (int fd)
+{
+  printf ("Reading ELF file\n");
+  Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor read-again: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  return elf;
+}
+
+static void
+check_section_size (Elf *elf, size_t size)
+{
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data 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 DATA section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (shdr->sh_size == size)
+    printf ("OK %zd bytes.\n", size);
+  else
+    {
+      printf ("BAD size, expected %zd, got %" PRIu64 "\n",
+	      size, shdr->sh_size);
+      exit (-1);
+    }
+}
+
+static void
+check_section_data (Elf *elf, char *data, size_t len, size_t times)
+{
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  Elf_Data *d = NULL;
+  for (size_t i = 0; i < times; i++)
+    {
+      if (d == NULL || i * len >= d->d_off + d->d_size)
+	{
+	  d = elf_getdata (scn, d);
+	  if (d == NULL)
+	    {
+	      printf ("cannot get data for section item %zd: %s\n",
+		      i, elf_errmsg (-1));
+	      exit (1);
+	    }
+	  else
+	    printf ("OK, section data item %zd (d_off: %zd, d_size: %zd)\n",
+		    i, d->d_off, d->d_size);
+	}
+      char *d_data = (char *) d->d_buf + (len * i) - d->d_off;
+      printf ("%zd data (d_off: %zd, len * i: %zd): (%p + %zd) %s\n",
+	      i, d->d_off, len * i, d->d_buf, (len * i) - d->d_off, d_data);
+      if (memcmp (data, d_data, len) != 0)
+	{
+	  printf ("Got bad data in section for item %zd.\n", i);
+	  exit (1);
+	}
+    }
+}
+
+static void
+check_elf (int class)
+{
+  static const char *fname;
+  fname = class == ELFCLASS32 ? "newdata.elf32" : "newdata.elf64";
+
+  printf ("check_elf: %s\n", fname);
+
+  int fd = open (fname, O_RDWR|O_CREAT|O_TRUNC, 00666);
+  if (fd == -1)
+    {
+      printf ("cannot create `%s': %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  Elf *elf = create_elf (fd, class);
+  check_section_size (elf, DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 1);
+
+  // Add some more data (won't be written to disk).
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 2);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data directly.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+  // But don't check contents, that would read the data...
+
+  // Add some more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 2);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 3);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data after raw reading.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+  // But don't check contents, that would read the data...
+
+  // Get raw data before adding new data.
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+    {
+      printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  printf ("elf_rawdata\n");
+  Elf_Data *data = elf_rawdata (scn, NULL);
+  if (data == NULL)
+    {
+      printf ("couldn't get raw data from section: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (data->d_size != DATA_LEN)
+    {
+      printf ("Unexpected Elf_Data: %zd", data->d_size);
+      exit (1);
+    }
+
+  // Now add more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 2);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 3);
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  // Read the ELF from disk now.  And add new data after data reading.
+  fd = open (fname, O_RDONLY);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = read_elf (fd);
+  check_section_size (elf, DATA_LEN);
+  // Get (converted) data before adding new data.
+  check_section_data (elf, DATA, DATA_LEN, 1);
+
+  printf ("elf_getdata\n");
+
+  // Now add more data.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 2 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 2);
+
+  // And some more.
+  add_section_data (elf, DATA, DATA_LEN);
+  check_section_size (elf, 3 * DATA_LEN);
+  check_section_data (elf, DATA, DATA_LEN, 3);
+
+  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)))
+{
+  // Initialize libelf.
+  elf_version (EV_CURRENT);
+
+  // Fill holes with something non-zero to more easily spot bad data.
+  elf_fill ('X');
+
+  check_elf (ELFCLASS32);
+  check_elf (ELFCLASS64);
+
+  return 0;
+}
-- 
1.8.3.1