public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
@ 2019-06-19  0:04 Mark Wielaard
  2019-06-19 23:10 ` Mark Wielaard
  2019-07-02 18:40 ` Lei Zhang
  0 siblings, 2 replies; 12+ messages in thread
From: Mark Wielaard @ 2019-06-19  0:04 UTC (permalink / raw)
  To: elfutils-devel
  Cc: fche, mcermak, dpranke, thestig, thomasanderson, Mark Wielaard

Hi,

I saw some curious crashes and broken ELF files in the Fedora builder
for really big packages (e.g. Qt5). It turned out that when libelf tries
to update or write out a 4GB+ files it used some 32bit offsets/types
where it should have been using 64bit offsets/types.

While searching for the bad coding pattern (using Elf64_Word as if it
was a 64bit type, it isn't!) I found that chromium probably is also
so big to hit this issue. There is even a somewhat similar fix!
https://chromium.googlesource.com/chromium/src/+/refs/heads/master/buildtools/third_party/eu-strip/fix-elf-size.patch

My apologies if you tried to upstream this and I missed it. But I think
the patch below is a more complete fix. If you could test it in your
setup that would be great.

The patch also contains a testcase. But since it is necessary to create
and process a 4GB+ file it is guarded by some checks to make sure the
machine has enough disk and memory. Do these checks look reasonable?
They probably prevent (make the test SKIP) on all buildbot CI workers.

Cheers,

Mark

-

Some years ago, in commit b1d0b0fc "libelf: Use int64_t for offsets in
libelf.h", we changed the public interface to use 64bit offsets/sizes.
This wasn't really a API change, before we relied on loff_t always
being 64bits on all platforms.

We didn't change the implementation to use the int64_t type though.
That was a little confusing, since the function definitions used a
different type, int64_t, from the function implementations, off_t.
Since we always build with _FILE_OFFSET_BITS=64 this should be fine.
But it was a bit sloppy and confusing.

Worse is that we got the translation of offset/sizes wrong in a
couple of places when translating to ELF types. In various places
we would use Elf32_Word or Elf64_Word. But both are 32bit (unsigned)
types! As is GElf_Word. Elf32_Off is 32bits and Elf64_Off is 64bits.
But we were not using those consistently.

This patch introduces comments for the usage of [G]Elf(32|64)Word in
libelf that are correct. And introduces Elf(32|64)_SizeWord in
elf32_updatenull.c where we want to make a difference between sizes
and offsets (the ELF variants are both unsigned, while int64_t/loff_t
is signed).

It also includes a new run-large-elf-file.sh test that creates a
large ELF files (one 64bit, little endian, rel and another big endian,
non-rel) and runs eu-strip, eu-elflint, eu-unstrip and eu-elfcmp.
Before this patch, that test case fails and creates corrupt ELF files.

The test is guarded by some checks that try to make sure there is
enough disk space and memory available on the machine. The test is
skipped otherwise.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog              | 32 ++++++++++++++
 libelf/common.h               |  2 +-
 libelf/elf32_newphdr.c        |  3 ++
 libelf/elf32_updatefile.c     |  8 ++--
 libelf/elf32_updatenull.c     | 34 ++++++++-------
 libelf/elf_begin.c            | 15 ++++---
 libelf/elf_getaroff.c         |  2 +-
 libelf/elf_getbase.c          |  4 +-
 libelf/elf_getdata_rawchunk.c |  2 +-
 libelf/elf_update.c           |  8 ++--
 libelf/libelfP.h              | 18 ++++----
 tests/ChangeLog               |  9 ++++
 tests/Makefile.am             |  2 +
 tests/addsections.c           | 44 +++++++++++++++----
 tests/run-large-elf-file.sh   | 99 +++++++++++++++++++++++++++++++++++++++++++
 15 files changed, 230 insertions(+), 52 deletions(-)
 create mode 100755 tests/run-large-elf-file.sh

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 82e18eb..dde6c81 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,35 @@
+2019-06-18  Mark Wielaard  <mark@klomp.org>
+
+	* common.h (allocate_elf): Use int64_t instead of off_t for offset.
+	* elf32_newphdr.c (newphdr): Document why Elf32/64_Word is correct.
+	* elf32_updatefile.c (fill): Use int64_t instead of off_t for pos.
+	(updatefile): Define last_offset, shdr_offset and scn_start as
+	int64_t instead of off_t.
+	* elf32_updatenull.c: Define Elf32_SizeWord and Elf64_SizeWord.
+	(updatenull_wrlock): Return int64_t instead of off_t. Define size,
+	sh_entsize, sh_align and sh_size as SizeWords. Define offset as
+	int64_t.  Cast data->d_off to SizeWord instead of GElf_Word. Drop
+	size GElf_Word cast. Cast offset to SizeWord instead of GElf_Word
+	when comparing with sh_size.
+	* elf_begin.c (get_shnum): Define offset as int64_t instead of
+	off_t. Document why use GElf_Word is correct.
+	(file_read_elf): Define offset as int64_t instead of off_t.
+	(__libelf_read_mmaped_file): Likewise.
+	(read_unmmaped_file): Likewise.
+	(read_file): Likewise.
+	* elf_getaroff.c (elf_getaroff): Return int64_t.
+	* elf_getbase.c (elf_getbase): Likewise.
+	* elf_getdata_rawchunk.c (elf_getdata_rawchunk): Define offset as
+	int64_t instead of off_t.
+	* elf_update.c (write_file): Return int64_t instead of off_t,
+	define size as int64_t instead of off_t.
+	(elf_update): Likewise.
+	* libelfP.h (struct Elf): Define start_offset, sizestr_offset and
+	offset as int64_t.
+	(__libelf_read_mmaped_file): Define offset as int64_t.
+	(__elf32_updatenull_wrlock): Return int64_t.
+	(__elf64_updatenull_wrlock): Return int64_t.
+
 2019-05-12  Mark Wielaard  <mark@klomp.org>
 
 	* elf32_updatenull.c (updatenull_wrlock): Mark shdr_flags dirty if
diff --git a/libelf/common.h b/libelf/common.h
index 6248690..b0175f6 100644
--- a/libelf/common.h
+++ b/libelf/common.h
@@ -68,7 +68,7 @@ determine_kind (void *buf, size_t len)
 /* Allocate an Elf descriptor and fill in the generic information.  */
 static inline Elf *
 __attribute__ ((unused))
-allocate_elf (int fildes, void *map_address, off_t offset, size_t maxsize,
+allocate_elf (int fildes, void *map_address, int64_t offset, size_t maxsize,
               Elf_Cmd cmd, Elf *parent, Elf_Kind kind, size_t extra)
 {
   Elf *result = (Elf *) calloc (1, sizeof (Elf) + extra);
diff --git a/libelf/elf32_newphdr.c b/libelf/elf32_newphdr.c
index 4aa7213..7dd78ca 100644
--- a/libelf/elf32_newphdr.c
+++ b/libelf/elf32_newphdr.c
@@ -56,6 +56,9 @@ elfw2(LIBELFBITS,newphdr) (Elf *elf, size_t count)
       return NULL;
     }
 
+  /* This check is correct, it is for sh_info, which is either
+     Elf32_Word or Elf64_Word, both being 32 bits.  But count is size_t
+     so might not fit on 32bit ELF files.  */
   if (unlikely ((ElfW2(LIBELFBITS,Word)) count != count))
     {
       __libelf_seterrno (ELF_E_INVALID_OPERAND);
diff --git a/libelf/elf32_updatefile.c b/libelf/elf32_updatefile.c
index eea51a7..f67e626 100644
--- a/libelf/elf32_updatefile.c
+++ b/libelf/elf32_updatefile.c
@@ -498,7 +498,7 @@ __elfw2(LIBELFBITS,updatemmap) (Elf *elf, int change_bo, size_t shnum)
 
 /* Helper function to write out fill bytes.  */
 static int
-fill (int fd, off_t pos, size_t len, char *fillbuf, size_t *filledp)
+fill (int fd, int64_t pos, size_t len, char *fillbuf, size_t *filledp)
 {
   size_t filled = *filledp;
   size_t fill_len = MIN (len, FILLBUFSIZE);
@@ -651,7 +651,7 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
 
   /* From now on we have to keep track of the last position to eventually
      fill the gaps with the prescribed fill byte.  */
-  off_t last_offset;
+  int64_t last_offset;
   if (elf->state.ELFW(elf,LIBELFBITS).phdr == NULL)
     last_offset = elf_typesize (LIBELFBITS, ELF_T_EHDR, 1);
   else
@@ -664,7 +664,7 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
 					+ sizeof (ElfW2(LIBELFBITS,Shdr)))))
 	return 1;
 
-      off_t shdr_offset = elf->start_offset + ehdr->e_shoff;
+      int64_t shdr_offset = elf->start_offset + ehdr->e_shoff;
 #undef shdr_fctp
 #define shdr_fctp __elf_xfctstom[ELFW(ELFCLASS, LIBELFBITS) - 1][ELF_T_SHDR]
 
@@ -712,7 +712,7 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
 	  if (shdr->sh_type == SHT_NOBITS)
 	    goto next;
 
-	  off_t scn_start = elf->start_offset + shdr->sh_offset;
+	  int64_t scn_start = elf->start_offset + shdr->sh_offset;
 	  Elf_Data_List *dl = &scn->data_list;
 	  bool scn_changed = false;
 
diff --git a/libelf/elf32_updatenull.c b/libelf/elf32_updatenull.c
index 303055a..5f3cdbf 100644
--- a/libelf/elf32_updatenull.c
+++ b/libelf/elf32_updatenull.c
@@ -45,6 +45,10 @@
 # define LIBELFBITS 32
 #endif
 
+/* Some fields contain 32/64 sizes.  We cannot use Elf32/64_Word for those,
+   since those are both 32bits.  Elf32/64_Xword is always 64bits.  */
+#define Elf32_SizeWord Elf32_Word
+#define Elf64_SizeWord Elf64_Xword
 
 
 static int
@@ -122,7 +126,7 @@ ELFW(default_ehdr,LIBELFBITS) (Elf *elf, ElfW2(LIBELFBITS,Ehdr) *ehdr,
 }
 
 
-off_t
+int64_t
 internal_function
 __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 {
@@ -137,7 +141,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
     return -1;
 
   /* At least the ELF header is there.  */
-  off_t size = elf_typesize (LIBELFBITS, ELF_T_EHDR, 1);
+  ElfW2(LIBELFBITS,SizeWord) size = elf_typesize (LIBELFBITS, ELF_T_EHDR, 1);
 
   /* Set the program header position.  */
   if (elf->state.ELFW(elf,LIBELFBITS).phdr == NULL)
@@ -152,7 +156,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 	{
 	  /* The user is supposed to fill out e_phoff.  Use it and
 	     e_phnum to determine the maximum extend.  */
-	  size = MAX ((size_t) size,
+	  size = MAX (size,
 		      ehdr->e_phoff
 		      + elf_typesize (LIBELFBITS, ELF_T_PHDR, phnum));
 	}
@@ -205,11 +209,11 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 	    {
 	      Elf_Scn *scn = &list->data[cnt];
 	      ElfW2(LIBELFBITS,Shdr) *shdr = scn->shdr.ELFW(e,LIBELFBITS);
-	      off_t offset = 0;
+	      int64_t offset = 0;
 
 	      assert (shdr != NULL);
-	      ElfW2(LIBELFBITS,Word) sh_entsize = shdr->sh_entsize;
-	      ElfW2(LIBELFBITS,Word) sh_align = shdr->sh_addralign ?: 1;
+	      ElfW2(LIBELFBITS,SizeWord) sh_entsize = shdr->sh_entsize;
+	      ElfW2(LIBELFBITS,SizeWord) sh_align = shdr->sh_addralign ?: 1;
 	      if (unlikely (! powerof2 (sh_align)))
 		{
 		  __libelf_seterrno (ELF_E_INVALID_ALIGN);
@@ -299,8 +303,8 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 			  /* The user specified the offset and the size.
 			     All we have to do is check whether this block
 			     fits in the size specified for the section.  */
-			  if (unlikely ((GElf_Word) (data->d_off
-						     + data->d_size)
+			  if (unlikely ((ElfW2(LIBELFBITS,SizeWord))
+					(data->d_off + data->d_size)
 					> shdr->sh_size))
 			    {
 			      __libelf_seterrno (ELF_E_SECTION_TOO_SMALL);
@@ -329,7 +333,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 
 	      if (elf->flags & ELF_F_LAYOUT)
 		{
-		  size = MAX ((GElf_Word) size,
+		  size = MAX (size,
 			      (shdr->sh_type != SHT_NOBITS
 			       ? shdr->sh_offset + shdr->sh_size : 0));
 
@@ -353,8 +357,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 
 		  size = (size + sh_align - 1) & ~(sh_align - 1);
 		  int offset_changed = 0;
-		  update_if_changed (shdr->sh_offset, (GElf_Word) size,
-				     offset_changed);
+		  update_if_changed (shdr->sh_offset, size, offset_changed);
 		  changed |= offset_changed;
 
 		  if (offset_changed && scn->data_list_rear == NULL)
@@ -367,7 +370,8 @@ __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,
+		  update_if_changed (shdr->sh_size,
+				     (ElfW2(LIBELFBITS,SizeWord)) offset,
 				     size_changed);
 		  changed |= size_changed;
 
@@ -384,7 +388,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 		  && (elf->flags & ELF_F_PERMISSIVE) == 0)
 		{
 		  /* For compressed sections check the uncompressed size.  */
-		  ElfW2(LIBELFBITS,Word) sh_size;
+		  ElfW2(LIBELFBITS,SizeWord) sh_size;
 		  if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
 		    sh_size = shdr->sh_size;
 		  else
@@ -418,7 +422,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 	  /* The user is supposed to fill out e_shoff.  Use it and
 	     e_shnum (or sh_size of the dummy, first section header)
 	     to determine the maximum extend.  */
-	  size = MAX ((GElf_Word) size,
+	  size = MAX (size,
 		      (ehdr->e_shoff
 		       + (elf_typesize (LIBELFBITS, ELF_T_SHDR, shnum))));
 	}
@@ -432,7 +436,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
 #define SHDR_ALIGN sizeof (ElfW2(LIBELFBITS,Off))
 	  size = (size + SHDR_ALIGN - 1) & ~(SHDR_ALIGN - 1);
 
-	  update_if_changed (ehdr->e_shoff, (GElf_Word) size, elf->flags);
+	  update_if_changed (ehdr->e_shoff, size, elf->flags);
 
 	  /* Account for the section header size.  */
 	  size += elf_typesize (LIBELFBITS, ELF_T_SHDR, shnum);
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 5d095ff..8107a10 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -71,8 +71,8 @@ file_read_ar (int fildes, void *map_address, off_t offset, size_t maxsize,
 
 
 static size_t
-get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
-	   size_t maxsize)
+get_shnum (void *map_address, unsigned char *e_ident, int fildes,
+	   int64_t offset, size_t maxsize)
 {
   size_t result;
   union
@@ -243,6 +243,9 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 		CONVERT (size);
 	    }
 
+	  /* Although sh_size is an Elf64_Xword and can contain a 64bit
+	     value, we only expect an 32bit value max.  GElf_Word is
+	     32bit unsigned.  */
 	  if (size > ~((GElf_Word) 0))
 	    {
 	      /* Invalid value, it is too large.  */
@@ -266,7 +269,7 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 /* Create descriptor for ELF file in memory.  */
 static Elf *
 file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
-	       off_t offset, size_t maxsize, Elf_Cmd cmd, Elf *parent)
+	       int64_t offset, size_t maxsize, Elf_Cmd cmd, Elf *parent)
 {
   /* Verify the binary is of the class we can handle.  */
   if (unlikely ((e_ident[EI_CLASS] != ELFCLASS32
@@ -531,7 +534,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
 
 Elf *
 internal_function
-__libelf_read_mmaped_file (int fildes, void *map_address,  off_t offset,
+__libelf_read_mmaped_file (int fildes, void *map_address,  int64_t offset,
 			   size_t maxsize, Elf_Cmd cmd, Elf *parent)
 {
   /* We have to find out what kind of file this is.  We handle ELF
@@ -564,7 +567,7 @@ __libelf_read_mmaped_file (int fildes, void *map_address,  off_t offset,
 
 
 static Elf *
-read_unmmaped_file (int fildes, off_t offset, size_t maxsize, Elf_Cmd cmd,
+read_unmmaped_file (int fildes, int64_t offset, size_t maxsize, Elf_Cmd cmd,
 		    Elf *parent)
 {
   /* We have to find out what kind of file this is.  We handle ELF
@@ -626,7 +629,7 @@ read_unmmaped_file (int fildes, off_t offset, size_t maxsize, Elf_Cmd cmd,
 
 /* Open a file for reading.  If possible we will try to mmap() the file.  */
 static struct Elf *
-read_file (int fildes, off_t offset, size_t maxsize,
+read_file (int fildes, int64_t offset, size_t maxsize,
 	   Elf_Cmd cmd, Elf *parent)
 {
   void *map_address = NULL;
diff --git a/libelf/elf_getaroff.c b/libelf/elf_getaroff.c
index 5b59203..5c102ad 100644
--- a/libelf/elf_getaroff.c
+++ b/libelf/elf_getaroff.c
@@ -38,7 +38,7 @@
 #include "libelfP.h"
 
 
-off_t
+int64_t
 elf_getaroff (Elf *elf)
 {
   /* Be gratious, the specs demand it.  */
diff --git a/libelf/elf_getbase.c b/libelf/elf_getbase.c
index 8ec5f87..4890d33 100644
--- a/libelf/elf_getbase.c
+++ b/libelf/elf_getbase.c
@@ -37,8 +37,8 @@
 #include "libelfP.h"
 
 
-off_t
+int64_t
 elf_getbase (Elf *elf)
 {
-  return elf == NULL ? (off_t) -1 : elf->start_offset;
+  return elf == NULL ? (int64_t) -1 : elf->start_offset;
 }
diff --git a/libelf/elf_getdata_rawchunk.c b/libelf/elf_getdata_rawchunk.c
index 6a13073..1072f7d 100644
--- a/libelf/elf_getdata_rawchunk.c
+++ b/libelf/elf_getdata_rawchunk.c
@@ -41,7 +41,7 @@
 #include "common.h"
 
 Elf_Data *
-elf_getdata_rawchunk (Elf *elf, off_t offset, size_t size, Elf_Type type)
+elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
 {
   if (unlikely (elf == NULL))
     return NULL;
diff --git a/libelf/elf_update.c b/libelf/elf_update.c
index 36997c2..9b8867c 100644
--- a/libelf/elf_update.c
+++ b/libelf/elf_update.c
@@ -40,8 +40,8 @@
 #include "libelfP.h"
 
 
-static off_t
-write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
+static int64_t
+write_file (Elf *elf, int64_t size, int change_bo, size_t shnum)
 {
   int class = elf->class;
 
@@ -164,11 +164,11 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
 }
 
 
-off_t
+int64_t
 elf_update (Elf *elf, Elf_Cmd cmd)
 {
   size_t shnum;
-  off_t size;
+  int64_t size;
   int change_bo = 0;
 
   if (cmd != ELF_C_NULL
diff --git a/libelf/libelfP.h b/libelf/libelfP.h
index 5134414..b55d5c4 100644
--- a/libelf/libelfP.h
+++ b/libelf/libelfP.h
@@ -296,7 +296,7 @@ struct Elf
   int fildes;
 
   /* Offset in the archive this file starts or zero.  */
-  off_t start_offset;
+  int64_t start_offset;
 
   /* Size of the file in the archive or the entire file size, or ~0
      for an (yet) unknown size.  */
@@ -350,7 +350,7 @@ struct Elf
       int ehdr_flags;		/* Flags (dirty) for ELF header.  */
       int phdr_flags;		/* Flags (dirty|malloc) for program header.  */
       int shdr_malloced;	/* Nonzero if shdr array was allocated.  */
-      off_t sizestr_offset;	/* Offset of the size string in the parent
+      int64_t sizestr_offset;	/* Offset of the size string in the parent
 				   if this is an archive member.  */
       Elf32_Ehdr ehdr_mem;	/* Memory used for ELF header when not
 				   mmaped.  */
@@ -375,7 +375,7 @@ struct Elf
       int ehdr_flags;		/* Flags (dirty) for ELF header.  */
       int phdr_flags;		/* Flags (dirty|malloc) for program header.  */
       int shdr_malloced;	/* Nonzero if shdr array was allocated.  */
-      off_t sizestr_offset;	/* Offset of the size string in the parent
+      int64_t sizestr_offset;	/* Offset of the size string in the parent
 				   if this is an archive member.  */
       Elf64_Ehdr ehdr_mem;	/* Memory used for ELF header when not
 				   mmaped.  */
@@ -392,7 +392,7 @@ struct Elf
       char *long_names;		/* If no index is available but long names
 				   are used this elements points to the data.*/
       size_t long_names_len;	/* Length of the long name table.  */
-      off_t offset;		/* Offset in file we are currently at.
+      int64_t offset;		/* Offset in file we are currently at.
 				   elf_next() advances this to the next
 				   member of the archive.  */
       Elf_Arhdr elf_ar_hdr;	/* Structure returned by 'elf_getarhdr'.  */
@@ -445,7 +445,7 @@ extern Elf_Type __libelf_data_type (Elf *elf, int sh_type, GElf_Xword align)
 
 /* Create Elf descriptor from memory image.  */
 extern Elf *__libelf_read_mmaped_file (int fildes, void *map_address,
-				       off_t offset, size_t maxsize,
+				       int64_t offset, size_t maxsize,
 				       Elf_Cmd cmd, Elf *parent)
      internal_function;
 
@@ -467,10 +467,10 @@ extern int __libelf_set_rawdata_wrlock (Elf_Scn *scn) internal_function;
 
 
 /* Helper functions for elf_update.  */
-extern off_t __elf32_updatenull_wrlock (Elf *elf, int *change_bop,
-					size_t shnum) internal_function;
-extern off_t __elf64_updatenull_wrlock (Elf *elf, int *change_bop,
-					size_t shnum) internal_function;
+extern int64_t __elf32_updatenull_wrlock (Elf *elf, int *change_bop,
+					  size_t shnum) internal_function;
+extern int64_t __elf64_updatenull_wrlock (Elf *elf, int *change_bop,
+					  size_t shnum) internal_function;
 
 extern int __elf32_updatemmap (Elf *elf, int change_bo, size_t shnum)
      internal_function;
diff --git a/tests/ChangeLog b/tests/ChangeLog
index e038793..9d15f8f 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,12 @@
+2019-06-18  Mark Wielaard  <mark@klomp.org>
+
+	* Makefile.am (TESTS): Add run-large-elf-file.sh.
+	(EXTRA_DIST): Likewise.
+	* addsections.c (add_sections): Add sec_size argument, use it
+	as the size of the section data.
+	(main): Handle extra sec_size argument. Pass to add_sections.
+	* run-large-elf-file.sh: New test.
+
 2019-06-03  Mark Wielaard  <mark@klomp.org>
 
 	* elfcopy.c (copy_elf): When swapping the sh_offsets of two sections,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87428aa..3d95cf6 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -83,6 +83,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \
 	run-next-files.sh run-next-lines.sh \
 	run-get-pubnames.sh run-get-aranges.sh run-allfcts.sh \
 	run-show-abbrev.sh run-line2addr.sh hash \
+	run-large-elf-file.sh \
 	newscn run-strip-test.sh run-strip-test2.sh \
 	run-strip-test3.sh run-strip-test4.sh run-strip-test5.sh \
 	run-strip-test6.sh run-strip-test7.sh run-strip-test8.sh \
@@ -422,6 +423,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     testfile-riscv64-core.bz2 \
 	     run-reverse-sections.sh run-reverse-sections-self.sh \
 	     run-copyadd-sections.sh run-copymany-sections.sh \
+	     run-large-elf-file.sh \
 	     run-typeiter-many.sh run-strip-test-many.sh \
 	     testfile-debug-rel-ppc64-g.o.bz2 \
 	     testfile-debug-rel-ppc64-z.o.bz2 \
diff --git a/tests/addsections.c b/tests/addsections.c
index cc8d065..c1b0fa8 100644
--- a/tests/addsections.c
+++ b/tests/addsections.c
@@ -70,9 +70,9 @@ setshstrndx (Elf *elf, size_t ndx)
 /* Will add nr new '.extra' sections and a new '.new_shstrtab' section
    at the end.  */
 static void
-add_sections (const char *name, size_t nr, int use_mmap)
+add_sections (const char *name, size_t nr, int use_mmap, size_t sec_size)
 {
-  printf ("add_sections '%s': %zd\n", name, nr);
+  printf ("add_sections '%s': %zd (sec_size: %zd)\n", name, nr, sec_size);
 
   int fd = open (name, O_RDWR);
   if (fd < 0)
@@ -149,6 +149,25 @@ add_sections (const char *name, size_t nr, int use_mmap)
       exit (1);
     }
 
+  void *buf;
+  size_t bufsz;
+  if (sec_size == 0)
+    {
+      buf = strdup ("extra");
+      bufsz = strlen ("extra") + 1;
+    }
+  else
+    {
+      buf = malloc (sec_size);
+      if (buf == NULL)
+	{
+	  printf ("cannot allocate buffer data of %zd bytes\n", sec_size);
+	  exit (1);
+	}
+      memset (buf, 0xAA, sec_size);
+      bufsz = sec_size;
+    }
+
   // Add lots of .extra sections...
   size_t cnt = 0;
   while (cnt++ < nr)
@@ -169,8 +188,8 @@ add_sections (const char *name, size_t nr, int use_mmap)
 	  exit (1);
 	}
 
-      data->d_size = strlen ("extra") + 1;
-      data->d_buf = "extra";
+      data->d_size = bufsz;
+      data->d_buf = buf;
       data->d_type = ELF_T_BYTE;
       data->d_align = 1;
 
@@ -274,6 +293,7 @@ add_sections (const char *name, size_t nr, int use_mmap)
       exit (1);
     }
 
+  free (buf);
   free (new_shstrtab_buf);
 }
 
@@ -282,10 +302,11 @@ main (int argc, char *argv[])
 {
   elf_version (EV_CURRENT);
 
-  /* Takes the given file, and adds the given number of sections.  */
-  if (argc < 3 || argc > 4)
+  /* Takes the given file, and adds the given number of sections.
+     Optionally using mmap and optionally using a given section size.  */
+  if (argc < 3 || argc > 5)
     {
-      fprintf (stderr, "addsections [--mmap] nr elf.file\n");
+      fprintf (stderr, "addsections [--mmap] nr elf.file [sec_size]\n");
       exit (1);
     }
 
@@ -298,8 +319,13 @@ main (int argc, char *argv[])
     }
 
   size_t nr = atoi (argv[argn++]);
-  const char *file = argv[argn];
-  add_sections (file, nr, use_mmap);
+  const char *file = argv[argn++];
+
+  size_t sec_size = 0;
+  if (argn < argc)
+    sec_size = atol (argv[argn++]);
+
+  add_sections (file, nr, use_mmap, sec_size);
 
   return 0;
 }
diff --git a/tests/run-large-elf-file.sh b/tests/run-large-elf-file.sh
new file mode 100755
index 0000000..3d1bdb6
--- /dev/null
+++ b/tests/run-large-elf-file.sh
@@ -0,0 +1,99 @@
+#! /bin/bash
+# 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
+
+# Only run on 64bit systems, 32bit systems don't support > 4GB
+# ELF files.
+long_bit=$(getconf LONG_BIT)
+echo "long_bit: $long_bit"
+if test $long_bit -ne 64; then
+  echo "Only 64bit systems can create > 4GB ELF files"
+  exit 77
+fi
+
+# These tests need lots of disk space since they test files > 4GB.
+# Skip if there just isn't enough (2.5 * 4 = 10GB).
+space_available=$[$(stat -f --format="%a*%S" .)/(1024 * 1024 * 1024)]
+echo "space_available: $space_available"
+if test $space_available -lt 10; then
+  echo "Not enough disk space, need at least 10GB available"
+  exit 77
+fi
+
+# Make sure the files fit into memory, assume 6GB needed (2.5 * 2 + 1 extra).
+mem_available=$(free -g | grep ^Mem: | awk -F ' +' '{print $7}')
+echo "mem_available: $mem_available"
+if test $mem_available -lt 6; then
+  echo "Need at least 6GB free available memory"
+  exit 77
+fi
+
+# NOTE: test file will be mangled and removed!
+test_file ()
+{
+  in_file="$1"
+  readelf_out="${in_file}.readelf.out"
+  out_file_strip="${in_file}.strip"
+  out_file_debug="${in_file}.debug"
+
+  testfiles ${in_file}
+  tempfiles ${readelf_out} ${out_file_mmap} ${out_file_strip} ${out_file_debug}
+
+  # Add two 2GB sections to the file.
+  echo "addsections 2 ${in_file} 2147483648"
+  testrun ${abs_builddir}/addsections 2 ${in_file} 2147483648
+  testrun ${abs_top_builddir}/src/readelf -S ${in_file} > ${readelf_out}
+  nr=$(grep '.extra' ${readelf_out} | wc -l)
+  if test ${nr} != 2; then
+    # Show what went wrong
+    cat ${readelf_out}
+    exit 1
+  fi
+
+  echo "strip -o ${out_file_strip} -f ${out_file_debug} ${in_file}"
+  testrun ${abs_top_builddir}/src/strip -o ${out_file_strip} \
+                                        -f ${out_file_debug} ${in_file}
+
+  echo "elflint --gnu ${out_file_strip}"
+  testrun ${abs_top_builddir}/src/elflint --gnu ${out_file_strip}
+
+  echo "elflint --gnu -d ${out_file_debug}"
+  testrun ${abs_top_builddir}/src/elflint --gnu -d ${out_file_debug}
+
+  # Now test unstrip recombining those files.
+  echo "unstrip ${out_file_strip} ${out_file_debug}"
+  testrun ${abs_top_builddir}/src/unstrip ${out_file_strip} ${out_file_debug}
+
+  echo "elfcmp ${out_file} ${out_file_strip}"
+  testrun ${abs_top_builddir}/src/elfcmp ${in_file} ${out_file_debug}
+
+  # Remove the temp files immediately, they are big...
+  rm -f ${in_file} ${out_file_strip} ${out_file_debug}
+}
+
+# A collection of random testfiles to test 64bit, little/big endian
+# and non-ET_REL (with phdrs)/ET_REL (without phdrs).
+# Don't test 32bit, they cannot go beyond 4GB.
+
+# 64bit, little endian, rel
+test_file testfile38
+
+# 64bit, big endian, non-rel
+test_file testfile27
+
+exit 0
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-06-19  0:04 [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files Mark Wielaard
@ 2019-06-19 23:10 ` Mark Wielaard
  2019-06-20  1:54   ` Dmitry V. Levin
  2019-07-02 18:40 ` Lei Zhang
  1 sibling, 1 reply; 12+ messages in thread
From: Mark Wielaard @ 2019-06-19 23:10 UTC (permalink / raw)
  To: elfutils-devel; +Cc: fche, mcermak, dpranke, thestig, thomasanderson

On Wed, 2019-06-19 at 02:04 +0200, Mark Wielaard wrote:
> The patch also contains a testcase. But since it is necessary to create
> and process a 4GB+ file it is guarded by some checks to make sure the
> machine has enough disk and memory. Do these checks look reasonable?
> They probably prevent (make the test SKIP) on all buildbot CI workers.

We discussed this a bit on irc and it was pointed out that the machine
also really needs to have a somewhat fast disk. After a bit of
experimenting I found the following the be a good indicator of the
testcase being able to run in reasonable time:

diff --git a/tests/run-large-elf-file.sh b/tests/run-large-elf-file.sh
index 3d1bdb6..d97eec9 100755
--- a/tests/run-large-elf-file.sh
+++ b/tests/run-large-elf-file.sh
@@ -35,6 +35,15 @@ if test $space_available -lt 10; then
   exit 77
 fi
 
+# Make sure the disk is reasonably fast, should be able to write 100MB/s
+fast_disk=1
+timeout -s9 10s dd conv=fsync if=/dev/urandom of=tempfile bs=1M count=1K \
+  || fast_disk=0; rm tempfile
+if test $fast_disk -eq 0; then
+  echo "Disk not fast enough, need at least 100MB/s"
+  exit 77
+fi
+
 # Make sure the files fit into memory, assume 6GB needed (2.5 * 2 + 1 extra).
 mem_available=$(free -g | grep ^Mem: | awk -F ' +' '{print $7}')
 echo "mem_available: $mem_available"

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-06-19 23:10 ` Mark Wielaard
@ 2019-06-20  1:54   ` Dmitry V. Levin
  2019-06-20  7:29     ` Mark Wielaard
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry V. Levin @ 2019-06-20  1:54 UTC (permalink / raw)
  To: Mark Wielaard
  Cc: elfutils-devel, fche, mcermak, dpranke, thestig, thomasanderson

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

On Thu, Jun 20, 2019 at 01:10:53AM +0200, Mark Wielaard wrote:
> On Wed, 2019-06-19 at 02:04 +0200, Mark Wielaard wrote:
> > The patch also contains a testcase. But since it is necessary to create
> > and process a 4GB+ file it is guarded by some checks to make sure the
> > machine has enough disk and memory. Do these checks look reasonable?
> > They probably prevent (make the test SKIP) on all buildbot CI workers.
> 
> We discussed this a bit on irc and it was pointed out that the machine
> also really needs to have a somewhat fast disk. After a bit of
> experimenting I found the following the be a good indicator of the
> testcase being able to run in reasonable time:
> 
> diff --git a/tests/run-large-elf-file.sh b/tests/run-large-elf-file.sh
> index 3d1bdb6..d97eec9 100755
> --- a/tests/run-large-elf-file.sh
> +++ b/tests/run-large-elf-file.sh
> @@ -35,6 +35,15 @@ if test $space_available -lt 10; then
>    exit 77
>  fi
>  
> +# Make sure the disk is reasonably fast, should be able to write 100MB/s
> +fast_disk=1
> +timeout -s9 10s dd conv=fsync if=/dev/urandom of=tempfile bs=1M count=1K \
> +  || fast_disk=0; rm tempfile

Why /dev/urandom?  I suggest to use /dev/zero instead.
Also, the check itself is quite expensive, it writes 1G into tempfile,
I suggest to move it after mem_available check.

> +if test $fast_disk -eq 0; then
> +  echo "Disk not fast enough, need at least 100MB/s"

It isn't necessarily a disk, I'd say that file system is not fast enough.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-06-20  1:54   ` Dmitry V. Levin
@ 2019-06-20  7:29     ` Mark Wielaard
  2019-06-28 21:38       ` Mark Wielaard
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Wielaard @ 2019-06-20  7:29 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: elfutils-devel, fche, mcermak, dpranke, thestig, thomasanderson

On Thu, 2019-06-20 at 04:54 +0300, Dmitry V. Levin wrote:
> On Thu, Jun 20, 2019 at 01:10:53AM +0200, Mark Wielaard wrote:
> > +# Make sure the disk is reasonably fast, should be able to write
> > 100MB/s
> > +fast_disk=1
> > +timeout -s9 10s dd conv=fsync if=/dev/urandom of=tempfile bs=1M
> > count=1K \
> > +  || fast_disk=0; rm tempfile
> 
> Why /dev/urandom?  I suggest to use /dev/zero instead.

Good question. In early testing I noticed some file systems seemed to
optimize away the whole writing of zeros and dd would finish almost
immediately. So I used /dev/urandom to get some "real bits" in the
file. But even that didn't always show the true write-through speed.
Then I added conv=fsync which makes sure to physically write output
file data before finishing. That seems to work to see the actual write
speed with either /dev/urandom or /dev/zero. So, I'll change it to
/dev/zero again. Thanks.

> Also, the check itself is quite expensive, it writes 1G into
> tempfile,
> I suggest to move it after mem_available check.

OK. Moved.

> > +if test $fast_disk -eq 0; then
> > +  echo "Disk not fast enough, need at least 100MB/s"
> 
> It isn't necessarily a disk, I'd say that file system is not fast
> enough.

Changed.

Thanks,

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-06-20  7:29     ` Mark Wielaard
@ 2019-06-28 21:38       ` Mark Wielaard
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Wielaard @ 2019-06-28 21:38 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: elfutils-devel, fche, mcermak, dpranke, thestig, thomasanderson

Hi,

I pushed this to master with the changes suggested by Dmitry.

Thanks,

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-06-19  0:04 [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files Mark Wielaard
  2019-06-19 23:10 ` Mark Wielaard
@ 2019-07-02 18:40 ` Lei Zhang
  2019-07-02 23:15   ` Mark Wielaard
  1 sibling, 1 reply; 12+ messages in thread
From: Lei Zhang @ 2019-07-02 18:40 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

On Tue, Jun 18, 2019 at 5:04 PM Mark Wielaard <mark@klomp.org> wrote:
> My apologies if you tried to upstream this and I missed it. But I think
> the patch below is a more complete fix. If you could test it in your
> setup that would be great.

Hi Mark,

I'm not sure if we tried to upstream the patch. So no worries there.

I tested and found some problems. My test procedure is to:
- Build elfutils at commit 31c8b3f098b0654db8f573b2a15d5b6d07d4d3b0
- Replace Chromium's buildtools/third_party/eu-strip/bin/eu-strip with
the newly built strip binary.
- Do an "official" Chromium build, with the following Chromium GN build config:

is_debug = false
is_official_build = true
strip_absolute_paths_from_debug_symbols = true
use_goma = true

This generates a 5.4 GB binary named "chrome" and then splits it into
"chrome.debug" and "chrome.stripped" using the strip command. Running
"objdump -x chrome.debug", I see the following in the "Dynamic
Section" output:

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .interp       0000001c  00000000000002e0  00000000000002e0  000002e0  2**0
                 ALLOC, READONLY
...
 40 .debug_loc    22f253c9  0000000000000000  0000000000000000  c8e11f1b  2**0
                 CONTENTS, READONLY, DEBUGGING
41 .debug_str    3176443a  0000000000000000  0000000000000000  ebd372e4  2**0
                 CONTENTS, READONLY, DEBUGGING
42 .debug_ranges 053cdc00  0000000000000000  0000000000000000  1d49b71e  2**0
                 CONTENTS, READONLY, DEBUGGING
43 .debug_macinfo 000064fb  0000000000000000  0000000000000000  2286931e  2**0
                 CONTENTS, READONLY, DEBUGGING
44 .debug_frame  011dfe98  0000000000000000  0000000000000000  2286f820  2**3
                 CONTENTS, READONLY, DEBUGGING
45 .gdb_index    24d27f19  0000000000000000  0000000000000000  23a4f6b8  2**0
                 CONTENTS, READONLY, DEBUGGING

Here, section 42 has the wrong file offset. It should be 0x11d49b71e,
since the file offset and size of section 41 is 0xebd372e4 +
0x3176443a. If I restore buildtools/third_party/eu-strip/bin/eu-strip
back to the original, and rebuild, then that generates the right
chrome.debug output.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-02 18:40 ` Lei Zhang
@ 2019-07-02 23:15   ` Mark Wielaard
  2019-07-02 23:21     ` Lei Zhang
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Wielaard @ 2019-07-02 23:15 UTC (permalink / raw)
  To: Lei Zhang; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

Hi,

On Tue, 2019-07-02 at 11:40 -0700, Lei Zhang wrote:
> I tested and found some problems. My test procedure is to:
> - Build elfutils at commit 31c8b3f098b0654db8f573b2a15d5b6d07d4d3b0
> - Replace Chromium's buildtools/third_party/eu-strip/bin/eu-strip with
> the newly built strip binary.
> - Do an "official" Chromium build, with the following Chromium GN build config:
> 
> is_debug = false
> is_official_build = true
> strip_absolute_paths_from_debug_symbols = true
> use_goma = true
> 
> This generates a 5.4 GB binary named "chrome" and then splits it into
> "chrome.debug" and "chrome.stripped" using the strip command. Running
> "objdump -x chrome.debug", I see the following in the "Dynamic
> Section" output:
> 
> Sections:
> Idx Name          Size      VMA               LMA               File off  Algn
>   0 .interp       0000001c  00000000000002e0  00000000000002e0  000002e0  2**0
>                  ALLOC, READONLY
> ...
>  40 .debug_loc    22f253c9  0000000000000000  0000000000000000  c8e11f1b  2**0
>                  CONTENTS, READONLY, DEBUGGING
> 41 .debug_str    3176443a  0000000000000000  0000000000000000  ebd372e4  2**0
>                  CONTENTS, READONLY, DEBUGGING
> 42 .debug_ranges 053cdc00  0000000000000000  0000000000000000  1d49b71e  2**0
>                  CONTENTS, READONLY, DEBUGGING
> 43 .debug_macinfo 000064fb  0000000000000000  0000000000000000  2286931e  2**0
>                  CONTENTS, READONLY, DEBUGGING
> 44 .debug_frame  011dfe98  0000000000000000  0000000000000000  2286f820  2**3
>                  CONTENTS, READONLY, DEBUGGING
> 45 .gdb_index    24d27f19  0000000000000000  0000000000000000  23a4f6b8  2**0
>                  CONTENTS, READONLY, DEBUGGING
> 
> Here, section 42 has the wrong file offset. It should be 0x11d49b71e,
> since the file offset and size of section 41 is 0xebd372e4 +
> 0x3176443a. If I restore buildtools/third_party/eu-strip/bin/eu-strip
> back to the original, and rebuild, then that generates the right
> chrome.debug output.

Thanks so much for testing. And sorry it didn't work.
It clearly is a 32bit issue, because the difference between the
expected and actually gotten value is clearly bit 32 being cleared.

I must have missed another 32bit type type or cast, but cannot find it
just by inspecting/reading the code. And I am not seeing it with the
largefile testcase I added. But that might be because it doesn't have
that many sections to begin with (just two large enough to push it over
the 4GB limit).

I'll try to create a testcase to replicate the issue to see if I can
debug where the offset value gets truncated.

Or do you happen to have the 5.4 GB binary named "chrome" create before
splitting still around somewhere where I could download it?

Thanks,

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-02 23:15   ` Mark Wielaard
@ 2019-07-02 23:21     ` Lei Zhang
  2019-07-03 14:53       ` Mark Wielaard
  0 siblings, 1 reply; 12+ messages in thread
From: Lei Zhang @ 2019-07-02 23:21 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

On Tue, Jul 2, 2019 at 4:15 PM Mark Wielaard <mark@klomp.org> wrote:
> I'll try to create a testcase to replicate the issue to see if I can
> debug where the offset value gets truncated.

Sounds good to me.

> Or do you happen to have the 5.4 GB binary named "chrome" create before
> splitting still around somewhere where I could download it?

I still have the file here. I'll send you a link in a separate email.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-02 23:21     ` Lei Zhang
@ 2019-07-03 14:53       ` Mark Wielaard
  2019-07-03 15:23         ` Lei Zhang
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Wielaard @ 2019-07-03 14:53 UTC (permalink / raw)
  To: Lei Zhang; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

Hi,

On Tue, 2019-07-02 at 16:21 -0700, Lei Zhang wrote:
> On Tue, Jul 2, 2019 at 4:15 PM Mark Wielaard <mark@klomp.org> wrote:
> > I'll try to create a testcase to replicate the issue to see if I can
> > debug where the offset value gets truncated.
> 
> Sounds good to me.
> 
> > Or do you happen to have the 5.4 GB binary named "chrome" create before
> > splitting still around somewhere where I could download it?
> 
> I still have the file here. I'll send you a link in a separate email.

Thanks for the file, it is indeed pretty big :)
But I am not able to replicate the issue with elfutils from git trunk.
commit 31c8b3f098b0654db8f573b2a15d5b6d07d4d3b0

If I do:
$ export LD_LIBRARY_PATH=/opt/local/install/elfutils/lib
$ /opt/local/install/elfutils/bin/eu-strip \
  -o chrome_elfutil_test.stripped \
  -f chrome_elfutil_test.debug chrome_elfutil_test

then the sections seem to come out correctly:

 36 .debug_info   bb8fa075  0000000000000000  0000000000000000  00000400  2**0
                  CONTENTS, READONLY, DEBUGGING
 37 .debug_abbrev 02e97444  0000000000000000  0000000000000000  bb8fa475  2**0
                  CONTENTS, READONLY, DEBUGGING
 38 .debug_aranges 00000540  0000000000000000  0000000000000000  be7918b9  2**0
                  CONTENTS, READONLY, DEBUGGING
 39 .debug_line   0a680122  0000000000000000  0000000000000000  be791df9  2**0
                  CONTENTS, READONLY, DEBUGGING
 40 .debug_loc    22f253c9  0000000000000000  0000000000000000  c8e11f1b  2**0
                  CONTENTS, READONLY, DEBUGGING
 41 .debug_str    3176443a  0000000000000000  0000000000000000  ebd372e4  2**0
                  CONTENTS, READONLY, DEBUGGING
 42 .debug_ranges 053cdc00  0000000000000000  0000000000000000  11d49b71e  2**0
                  CONTENTS, READONLY, DEBUGGING
 43 .debug_macinfo 000064fb  0000000000000000  0000000000000000  12286931e  2**0
                  CONTENTS, READONLY, DEBUGGING
 44 .debug_frame  011dfe98  0000000000000000  0000000000000000  12286f820  2**3
                  CONTENTS, READONLY, DEBUGGING
 45 .gdb_index    24d27f19  0000000000000000  0000000000000000  123a4f6b8  2**0
                  CONTENTS, READONLY, DEBUGGING

And both the produced chrome_elfutil_test.stripped and
chrome_elfutil_test.debug files seem valid ELF files.

But you might not be using the upstream build system, and you might use
different flags to call it. So, two questions. How did you build your
eu-strip binary? And how do you invoke it?

Thanks,

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-03 14:53       ` Mark Wielaard
@ 2019-07-03 15:23         ` Lei Zhang
  2019-07-03 15:34           ` Mark Wielaard
  0 siblings, 1 reply; 12+ messages in thread
From: Lei Zhang @ 2019-07-03 15:23 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

On Wed, Jul 3, 2019 at 7:53 AM Mark Wielaard <mark@klomp.org> wrote:
> Thanks for the file, it is indeed pretty big :)
> But I am not able to replicate the issue with elfutils from git trunk.
> commit 31c8b3f098b0654db8f573b2a15d5b6d07d4d3b0
>
> And both the produced chrome_elfutil_test.stripped and
> chrome_elfutil_test.debug files seem valid ELF files.

I figured out the problem on my side. The strip binary dynamically
linked to /usr/lib/x86_64-linux-gnu/libelf.so.1 and friends. Once I
set LD_LIBRARY_PATH, I got the expected output.

> But you might not be using the upstream build system, and you might use
> different flags to call it. So, two questions. How did you build your
> eu-strip binary? And how do you invoke it?

I'm building at the same commit on 64-bit Linux:

git clone git://sourceware.org/git/elfutils.git
cd elfutils
autoreconf -i -f
./configure --enable-maintainer-mode
make
make check
cp ./src/strip /path/to/chrome/src/buildtools/third_party/eu-strip/bin/eu-strip

Then I did the build on the Chromium side, which essentially runs:

strip -o chrome.stripped -f chrome.debug chrome

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-03 15:23         ` Lei Zhang
@ 2019-07-03 15:34           ` Mark Wielaard
  2019-07-03 16:00             ` Lei Zhang
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Wielaard @ 2019-07-03 15:34 UTC (permalink / raw)
  To: Lei Zhang; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

On Wed, 2019-07-03 at 08:23 -0700, Lei Zhang wrote:
> On Wed, Jul 3, 2019 at 7:53 AM Mark Wielaard <mark@klomp.org> wrote:
> > Thanks for the file, it is indeed pretty big :)
> > But I am not able to replicate the issue with elfutils from git
> > trunk.
> > commit 31c8b3f098b0654db8f573b2a15d5b6d07d4d3b0
> > 
> > And both the produced chrome_elfutil_test.stripped and
> > chrome_elfutil_test.debug files seem valid ELF files.
> 
> I figured out the problem on my side. The strip binary dynamically
> linked to /usr/lib/x86_64-linux-gnu/libelf.so.1 and friends. Once I
> set LD_LIBRARY_PATH, I got the expected output.

Great. Thanks for testing on a real world binary.
There aren't actually that many out there this big!
Please do let me know if you still see issues (or discover others).

Thanks,

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files.
  2019-07-03 15:34           ` Mark Wielaard
@ 2019-07-03 16:00             ` Lei Zhang
  0 siblings, 0 replies; 12+ messages in thread
From: Lei Zhang @ 2019-07-03 16:00 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, fche, mcermak, Dirk Pranke, Thomas Anderson

On Wed, Jul 3, 2019 at 8:34 AM Mark Wielaard <mark@klomp.org> wrote:
> Great. Thanks for testing on a real world binary.
> There aren't actually that many out there this big!
> Please do let me know if you still see issues (or discover others).

You are welcome. Thank you for fixing this issue in elfutils.

We will probably rebuild our copy of eu-strip from upstream at some
point and stop carrying our own patch. Hopefully there won't be any
issues.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-07-03 16:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19  0:04 [PATCH] libelf: Fix some 32bit offset/size issues that break updating 4G+ files Mark Wielaard
2019-06-19 23:10 ` Mark Wielaard
2019-06-20  1:54   ` Dmitry V. Levin
2019-06-20  7:29     ` Mark Wielaard
2019-06-28 21:38       ` Mark Wielaard
2019-07-02 18:40 ` Lei Zhang
2019-07-02 23:15   ` Mark Wielaard
2019-07-02 23:21     ` Lei Zhang
2019-07-03 14:53       ` Mark Wielaard
2019-07-03 15:23         ` Lei Zhang
2019-07-03 15:34           ` Mark Wielaard
2019-07-03 16:00             ` Lei Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).