Re: elf_cntl(ELF_C_FDREAD) breaks elf64_getshdr() with non-mmap()ed ELF files

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2012-07-19 at 11:56 +0200, Mark Wielaard wrote:
> On Wed, 2012-07-18 at 16:17 -0700, Roland McGrath wrote:
> > This seems vaguely reasonable to me, but OTOH since in the mmap case we do
> > eagerly set up the shdr pointers in file_read_elf, I think it makes sense
> > to be consistent and do that in __libelf_readall too.  It should also save
> > a little memory to not malloc the ehdr and shdr copies.  That means that
> > __libelf_readall really ought to free the old ehdr copy (which is always
> > cached) and the shdr copies if they were cached before.  Most of the code
> > can just be broken out of file_read_elf into a subroutine,
> > e.g. __libelf_maybe_precache_headers.  __libelf_readall will just need to
> > free the old state and clear the shdr_malloced flag.
> 
> I thought about that, but was afraid about other (concurrent) users of
> the already allocated data if we freed it and swapped in a new mmaped
> version. So I decided to just keep what we have and only copy in the new
> shdr data in load_shdr_wrlock () when requested. Do you think the memory
> savings are significant enough to risk data races? It can probably be
> done safely, but does require careful analysis for IMHO little gain.

I decided to go with the original assert-relaxation approach and not do
the optimization at this time. I did add the suggestion to the TODO
file. Any volunteers? I did make all other changes suggested by Roland.
Patch as committed and pushed attached.

Cheers,

Mark

commit 030f626f41b5d1d563c357743cfec492d4fa69a5
Author: Mark Wielaard <mjw@redhat.com>
Date:   Wed Jul 18 13:14:30 2012 +0200

    elf_getshdr should work for elf->flags & ELF_F_MALLOCED.
    
    Reported-by: Nick Alcock <nix@esperi.org.uk>
    Signed-off-by: Mark Wielaard <mjw@redhat.com>

diff --git a/ChangeLog b/ChangeLog
index 2a9fcd8..5842492 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2012-07-24  Mark Wielaard  <mjw@redhat.com>
+
+	* TODO: Add note on shdrs after elf_cntl (ELF_C_FDREAD).
+
 2012-06-22  Mark Wielaard  <mjw@redhat.com>
 
 	* configure.ac: Set version to 0.154.
diff --git a/TODO b/TODO
index 0012a56..ad10a5e 100644
--- a/TODO
+++ b/TODO
@@ -29,6 +29,11 @@ Time-stamp: <2009-02-05 22:08:01 drepper>
    is allowed or the structure is aligned.  Use ELF_F_MALLOCED flag
    to differentiate.
 
+** shdrs after elf_cntl (ELF_C_FDREAD)
+
+   Similar to the above. After ELF_C_FDREAD the file is completely
+   in memory.  See also this mailing list thread:
+   https://fedorahosted.org/pipermail/elfutils-devel/2012-July/002368.html
 
 * libdw
 
diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 0a6bcba..8c9ff8b 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2012-07-19  Mark Wielaard  <mjw@redhat.com>
+
+	* elf32_getshdr.c (load_shdr_wrlock): Add elf->flags & ELF_F_MALLOCED
+	to asserts.
+
 2012-07-17  Petr Machata  <pmachata@redhat.com>
 
 	* elf32_xlatetom.c (elfw2(LIBELFBITS, xlatetom)): Do not check for
diff --git a/libelf/elf32_getshdr.c b/libelf/elf32_getshdr.c
index f0319cb..bd9340d 100644
--- a/libelf/elf32_getshdr.c
+++ b/libelf/elf32_getshdr.c
@@ -1,5 +1,5 @@
 /* Return section header.
-   Copyright (C) 1998, 1999, 2000, 2001, 2002, 2005, 2007, 2009 Red Hat, Inc.
+   Copyright (C) 1998-2002, 2005, 2007, 2009, 2012 Red Hat, Inc.
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 1998.
 
@@ -80,11 +80,14 @@ load_shdr_wrlock (Elf_Scn *scn)
       ElfW2(LIBELFBITS,Shdr) *notcvt;
 
       /* All the data is already mapped.  If we could use it
-	 directly this would already have happened.  */
+	 directly this would already have happened.  Unless
+	 we allocated the memory ourselves and the ELF_F_MALLOCED
+	 flag is set.  */
       void *file_shdr = ((char *) elf->map_address
 			 + elf->start_offset + ehdr->e_shoff);
 
-      assert (ehdr->e_ident[EI_DATA] != MY_ELFDATA
+      assert ((elf->flags & ELF_F_MALLOCED)
+	      || ehdr->e_ident[EI_DATA] != MY_ELFDATA
 	      || (! ALLOW_UNALIGNED
 		  && ((uintptr_t) file_shdr
 		      & (__alignof__ (ElfW2(LIBELFBITS,Shdr)) - 1)) != 0));
@@ -92,7 +95,7 @@ load_shdr_wrlock (Elf_Scn *scn)
       /* Now copy the data and at the same time convert the byte order.  */
       if (ehdr->e_ident[EI_DATA] == MY_ELFDATA)
 	{
-	  assert (! ALLOW_UNALIGNED);
+	  assert ((elf->flags & ELF_F_MALLOCED) || ! ALLOW_UNALIGNED);
 	  memcpy (shdr, file_shdr, size);
 	}
       else
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 9280028..6133048 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,5 +1,14 @@
 2012-07-19  Mark Wielaard  <mjw@redhat.com>
 
+	* Makefile.am (check_PROGRAMS): Add test-elf_cntl_gelf_getshdr.
+	(TESTS): Add run-elf_cntl_gelf_getshdr.sh.
+	(EXTRA_DIST): Likewise.
+	(test_elf_cntl_gelf_getshdr_LDADD): New.
+	test-elf_cntl_gelf_getshdr.c: New test program.
+	run-elf_cntl_gelf_getshdr.sh: New test script.
+
+2012-07-19  Mark Wielaard  <mjw@redhat.com>
+
 	* run-elflint-self.sh: runtests on ../backends/*so files.
 
 2012-07-19  Mark Wielaard  <mjw@redhat.com>
diff --git a/tests/Makefile.am b/tests/Makefile.am
index fcbabf0..a8b0f16 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -50,7 +50,8 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  dwfl-addr-sect dwfl-bug-report early-offscn \
 		  dwfl-bug-getmodules dwarf-getmacros addrcfi \
 		  test-flag-nobits dwarf-getstring rerequest_tag \
-		  alldts md5-sha1-test typeiter low_high_pc
+		  alldts md5-sha1-test typeiter low_high_pc \
+		  test-elf_cntl_gelf_getshdr
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
 
@@ -81,7 +82,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-test-flag-nobits.sh run-prelink-addr-test.sh \
 	run-dwarf-getstring.sh run-rerequest_tag.sh run-typeiter.sh \
 	run-readelf-d.sh run-readelf-gdb_index.sh run-unstrip-n.sh \
-	run-low_high_pc.sh run-macro-test.sh
+	run-low_high_pc.sh run-macro-test.sh run-elf_cntl_gelf_getshdr.sh
 
 if !STANDALONE
 check_PROGRAMS += msg_tst md5-sha1-test
@@ -163,7 +164,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \
 	     testfilegdbindex7.bz2 \
 	     run-unstrip-n.sh testcore-rtlib.bz2 testcore-rtlib-ppc.bz2 \
 	     run-low_high_pc.sh testfile_low_high_pc.bz2 \
-	     run-macro-test.sh testfile-macinfo.bz2 testfile-macros.bz2
+	     run-macro-test.sh testfile-macinfo.bz2 testfile-macros.bz2 \
+	     run-elf_cntl_gelf_getshdr.sh
 
 installed_TESTS_ENVIRONMENT = libdir=$(DESTDIR)$(libdir) \
 			      bindir=$(DESTDIR)$(bindir) \
@@ -263,6 +265,7 @@ alldts_LDADD = $(libebl) $(libelf) $(libmudflap)
 md5_sha1_test_LDADD = $(libeu)
 typeiter_LDADD = $(libdw) $(libelf) $(libmudflap)
 low_high_pc_LDADD = $(libdw) $(libelf) $(libmudflap)
+test_elf_cntl_gelf_getshdr_LDADD = $(libelf) $(libmudflap)
 
 if GCOV
 check: check-am coverage
diff --git a/tests/run-elf_cntl_gelf_getshdr.sh b/tests/run-elf_cntl_gelf_getshdr.sh
new file mode 100755
index 0000000..41a7d15
--- /dev/null
+++ b/tests/run-elf_cntl_gelf_getshdr.sh
@@ -0,0 +1,30 @@
+#! /bin/sh
+# Copyright (C) 2012 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
+
+# However we open the elf file, the shdrs should be the same.
+testrun ./test-elf_cntl_gelf_getshdr READ test-elf_cntl_gelf_getshdr \
+  > test_shdr.out
+
+testrun_compare ./test-elf_cntl_gelf_getshdr MMAP test-elf_cntl_gelf_getshdr \
+  < test_shdr.out
+
+testrun_compare ./test-elf_cntl_gelf_getshdr FDREAD test-elf_cntl_gelf_getshdr \
+  < test_shdr.out
+
+rm -f test_shdr.out
diff --git a/tests/test-elf_cntl_gelf_getshdr.c b/tests/test-elf_cntl_gelf_getshdr.c
new file mode 100644
index 0000000..b561b53
--- /dev/null
+++ b/tests/test-elf_cntl_gelf_getshdr.c
@@ -0,0 +1,103 @@
+/* Copyright (C) 2012 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/>.  */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+#include <gelf.h>
+#include <libelf.h>
+#include <stdbool.h>
+#include <inttypes.h>
+
+int
+main (int argc, char *argv[])
+{
+  if (argc != 3)
+    {
+      fprintf (stderr, "Needs two arguments.\n");
+      fprintf (stderr, "First needs to be 'READ', 'MMAP' or 'FDREAD'\n");
+      fprintf (stderr, "Second is the ELF file to read.\n");
+      exit (2); /* user error */
+    }
+
+  bool do_mmap = false;
+  bool close_fd = false;
+  if (strcmp (argv[1], "READ") == 0)
+    {
+      do_mmap = false;
+      close_fd = false;
+    }
+  else if (strcmp (argv[1], "MMAP") == 0)
+    {
+      do_mmap = true;
+      close_fd = false;
+    }
+  else if  (strcmp (argv[1], "FDREAD") == 0)
+    {
+      do_mmap = false;
+      close_fd = true;
+    }
+  else
+    {
+      fprintf (stderr, "First arg needs to be 'READ', 'MMAP' or 'FDREAD'\n");
+      exit (2); /* user error */
+    }
+
+  elf_version (EV_CURRENT);
+
+  int fd = open (argv[2], O_RDONLY);
+  if (fd < 0)
+    {
+      fprintf (stderr, "Cannot open input file %s: %s\n", argv[2],
+	       strerror (errno));
+      exit (2);
+    }
+
+  Elf *elf = elf_begin (fd, do_mmap ? ELF_C_READ_MMAP : ELF_C_READ, NULL);
+  if (elf == NULL)
+    {
+      fprintf (stderr, "elf_begin failed for %s: %s\n", argv[2],
+	       elf_errmsg (-1));
+      exit (2);
+    }
+
+  if (! do_mmap && close_fd)
+    {
+      if (elf_cntl (elf, ELF_C_FDREAD) < 0)
+	{
+	  fprintf (stderr, "elf_cntl failed for %s: %s\n", argv[2],
+		   elf_errmsg (-1));
+	  exit (1);
+	}
+      close (fd);
+    }
+
+  Elf_Scn *scn = NULL;
+  while ((scn = elf_nextscn (elf, scn)) != NULL)
+    {
+      GElf_Shdr shdr_mem;
+      GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+      printf ("Section at offset %#0" PRIx64 "\n", shdr->sh_offset);
+    }
+
+  elf_end (elf);
+  exit (0);
+}