[PATCH] Ignore sh_offset of NOBITS sections

[PATCH] Ignore sh_offset of NOBITS sections

[Tom de Vries]

Hi,

Consider a hello world with debug info.  If we split off the debug info using
objcopy, we run into the following dwz failure:
...
$ gcc hello.c -g
$ objcopy --only-keep-debug a.out a.debug
$ dwz a.debug
dwz: Allocatable section in a.debug after non-allocatable ones
...

Dwz is complaining about section 19, which has an sh_offset of 0xe00:
...
  [19] .init_array       NOBITS           0000000000600e00  00000e00
       0000000000000008  0000000000000008  WA       0     0     8
...
which is larger than the sh_offset of 0x410 of section 28:
...
  [28] .debug_info       PROGBITS         0000000000000000  00000410
       00000000000005bb  0000000000000000           0     0     1...
...
However, the sh_offset field is meaningless for NOBITS sections, so the error
is unnecessary.

Fix the error by ignoring NOBITS sections in write_dso when:
- sanity-checking the section header table, and
- updating the offsets of sections and section header table to honour
  required aligmment.

In order to ensure that the sanity-checking of the section header table is not
too much weakened by skipping the NOBITS sections, add verify_alloc_sections,
which checks that:
- there are no allocatable sections after non-allocatable ones
  (after in terms of section header table order)
- the allocatable sections in the section header table are monotonically
  increasing in sh_addr
- the allocatable sections in the section header table are monotonically
  increasing in sh_offset (this is not guaranteed for NOBITS sections, but
  seems to be respected by objcopy --only-keep-debug).

Finally, ensure that sh_offset for NOBITS sections are unmodified by dwz,
which seems a more sane approach than trying to update values that are
potentially undefined.

OK for trunk?

Thanks,
- Tom

Ignore sh_offset of NOBITS sections

2019-03-12  Tom de Vries  <tdevries@suse.de>

	PR dwz/24251
	* dwz.c (verify_alloc_sections): New function.
	(write_dso): Ignore sh_offset of NOBITS sections.
	* testsuite/dwz.tests/objcopy-eu-unstrip-multifile.sh: New test.
	* testsuite/dwz.tests/objcopy-eu-unstrip.sh: New test.

---
 dwz.c                                              | 76 +++++++++++++++++++++-
 .../dwz.tests/objcopy-eu-unstrip-multifile.sh      | 24 +++++++
 testsuite/dwz.tests/objcopy-eu-unstrip.sh          | 20 ++++++
 3 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/dwz.c b/dwz.c
index 476807a..dffab77 100644
--- a/dwz.c
+++ b/dwz.c
@@ -9968,6 +9968,57 @@ read_dwarf (DSO *dso, bool quieter)
   return read_debug_info (dso, DEBUG_INFO);
 }
 
+/* Verify properties of SHF_ALLOC sections in the section header table.  */
+static int
+verify_alloc_sections (DSO *dso)
+{
+  int i;
+  int prev_alloc, update_prev_alloc;
+
+  prev_alloc = -1;
+  for (i = 1; i < dso->ehdr.e_shnum; ++i, prev_alloc = update_prev_alloc)
+    {
+      update_prev_alloc = prev_alloc;
+      if ((dso->shdr[i].sh_flags & SHF_ALLOC) == 0)
+	continue;
+      update_prev_alloc = i;
+
+      if (/* First allocatable section is not first section.  */
+	  (prev_alloc == -1 && i != 1)
+	  /* Previous allocatable section is not immediately preceding
+	     section.  */
+	  || (prev_alloc != -1 && prev_alloc != i - 1))
+	{
+	  error (0, 0, "Allocatable section in %s after non-allocatable "
+		 "ones", dso->filename);
+	  return 1;
+	}
+
+      if (prev_alloc == -1)
+	continue;
+
+      if (!(dso->shdr[prev_alloc].sh_addr <= dso->shdr[i].sh_addr))
+	{
+	  error (0, 0, "Allocatable sections in %s not monotonically "
+		 "increasing in sh_addr", dso->filename);
+	  return 1;
+	}
+
+      /* Even if sh_offset has no meaning for NOBITS sections, objcopy
+	 --only-keep-debug seems to generate monotonically increasing sh_offset
+	 for allocatable sections, so don't exclude NOBITS sections from the
+	 verification here.  */
+      if (!(dso->shdr[prev_alloc].sh_offset <= dso->shdr[i].sh_offset))
+	{
+	  error (0, 0, "Allocatable sections in %s not monotonically "
+		 "increasing in sh_offset", dso->filename);
+	  return 1;
+	}
+    }
+
+  return 0;
+}
+
 /* Open an ELF file NAME.  */
 static DSO *
 fdopen_dso (int fd, const char *name)
@@ -10056,6 +10107,10 @@ write_dso (DSO *dso, const char *file, struct stat *st)
   GElf_Word shstrtabadd = 0;
   char *shstrtab = NULL;
   bool remove_sections[SECTION_COUNT];
+  GElf_Off old_sh_offset[dso->ehdr.e_shnum];
+
+  for (i = 1; i < dso->ehdr.e_shnum; ++i)
+    old_sh_offset[i] = dso->shdr[i].sh_offset;
 
   memset (remove_sections, '\0', sizeof (remove_sections));
   ehdr = dso->ehdr;
@@ -10146,11 +10201,21 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 	  }
       }
 
+  /* The value of sh_offset for NOBITS sections is meaningless.  Reset to the
+     original value.  */
+  for (i = 1; i < dso->ehdr.e_shnum; ++i)
+    if (dso->shdr[i].sh_type == SHT_NOBITS)
+      dso->shdr[i].sh_offset = old_sh_offset[i];
+
+  if (verify_alloc_sections (dso))
+    return 1;
+
   if (min_shoff != ~(GElf_Off) 0)
     {
       for (j = 1; j < dso->ehdr.e_shnum; ++j)
 	if (dso->shdr[j].sh_offset >= min_shoff
 	    && dso->shdr[j].sh_addralign > 1
+	    && dso->shdr[j].sh_type != SHT_NOBITS
 	    && (dso->shdr[j].sh_offset & (dso->shdr[j].sh_addralign - 1)) != 0)
 	  break;
       if (j < dso->ehdr.e_shnum
@@ -10166,10 +10231,11 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 	  for (j = 1; j < dso->ehdr.e_shnum; ++j)
 	    if (dso->shdr[j].sh_offset < min_shoff && !last_shoff)
 	      continue;
+	    else if (dso->shdr[j].sh_type == SHT_NOBITS)
+	      continue;
 	    else if ((dso->shdr[j].sh_flags & SHF_ALLOC) != 0)
 	      {
-		error (0, 0, "Allocatable section in %s after non-allocatable "
-			     "ones", dso->filename);
+		error (0, 0, "Can't update allocatable section in %s", dso->filename);
 		return 1;
 	      }
 	    else if (dso->shdr[j].sh_offset < last_shoff)
@@ -10201,6 +10267,8 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 		}
 	      if (j == dso->ehdr.e_shnum)
 		break;
+	      if (dso->shdr[j].sh_type == SHT_NOBITS)
+		continue;
 	      dso->shdr[j].sh_offset = last_shoff;
 	      if (dso->shdr[j].sh_addralign > 1)
 		dso->shdr[j].sh_offset
@@ -10213,6 +10281,10 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 	}
     }
 
+  for (i = 1; i < dso->ehdr.e_shnum; ++i)
+    if (dso->shdr[i].sh_type == SHT_NOBITS)
+      assert (dso->shdr[i].sh_offset == old_sh_offset[i]);
+
   if (shstrtabadd != 0)
     {
       shstrtab = (char *) malloc (dso->shdr[dso->ehdr.e_shstrndx].sh_size);
diff --git a/testsuite/dwz.tests/objcopy-eu-unstrip-multifile.sh b/testsuite/dwz.tests/objcopy-eu-unstrip-multifile.sh
new file mode 100755
index 0000000..03ab852
--- /dev/null
+++ b/testsuite/dwz.tests/objcopy-eu-unstrip-multifile.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+set -e
+
+cp ../hello 1
+
+objcopy --only-keep-debug 1 1.debug
+objcopy --strip-debug 1 1.stripped
+
+cp 1.debug 2.debug
+
+dwz -m 3 1.debug 2.debug
+
+rm 2.debug
+
+eu-unstrip 1.stripped 1.debug -o 1.unstripped
+
+smaller-than.sh 1.unstripped 1
+
+ls=$(ls)
+ls=$(echo $ls)
+[ "$ls" = "1 1.debug 1.stripped 1.unstripped 3" ]
+
+rm -f 1 1.debugged 1.stripped 1.unstripped 3
diff --git a/testsuite/dwz.tests/objcopy-eu-unstrip.sh b/testsuite/dwz.tests/objcopy-eu-unstrip.sh
new file mode 100755
index 0000000..9fbb5cc
--- /dev/null
+++ b/testsuite/dwz.tests/objcopy-eu-unstrip.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+set -e
+
+cp ../hello 1
+
+objcopy --only-keep-debug 1 1.debug
+objcopy --strip-debug 1 1.stripped
+
+dwz 1.debug
+
+eu-unstrip 1.stripped 1.debug -o 1.unstripped
+
+smaller-than.sh 1.unstripped 1
+
+ls=$(ls)
+ls=$(echo $ls)
+[ "$ls" = "1 1.debug 1.stripped 1.unstripped" ]
+
+rm -f 1 1.debugged 1.stripped 1.unstripped