strip, unstrip: Handle SHT_GROUP correctly.

The usage of annobin in Fedora showed a couple of bugs when using
eu-strip and eu-unstrip on ET_REL files that contain multiple group
sections.

When stripping we should not remove the SHF_GROUP flag from sections
even if the group section itself might be removed. Either the section
itself gets removed, and so the flag doesn't matter. Or it gets moved
together with the group section into the debug file, and then it still
needs to have the flag set. Also we would "renumber" the section group
flag field (which isn't a section index, and so shouldn't be changed).

Often the group sections have the exact same name (".group"), flags
(none) and sometimes the same sizes. Which makes matching them hard.
Extract the group signature and compare those when comparing two
group sections.

Signed-off-by: Mark Wielaard <mark@klomp.org>

diff --git a/src/ChangeLog b/src/ChangeLog
index 7b59ed60b97e83b5bbeb7e3ce90a13227015a39d..d151e0d978c7cc82093df76ef3fd12debd0fc7a0 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,15 @@
+2018-10-12  Mark Wielaard  <mark@klomp.org>
+
+       * strip.c (handle_elf): Don't remove SHF_GROUP flag from sections.
+       Skip group section flag when renumbering section indexes.
+       * unstrip.c (struct section): Add sig field.
+       (compare_unalloc_sections): Take and use sig1 and sig2 as arguments.
+       (compare_sections): Pass signatures to compare_unalloc_sections.
+       (get_group_sig): New function.
+       (find_alloc_sections_prelink): Set signature.
+       (copy_elided_sections): Likewise and pass them on.
+       (find_unalloc_section): Take and pass signatures.
+
 2018-10-02  Andreas Schwab  <schwab@suse.de>
 
        * strip.c (handle_elf): Handle ADD/SUB relocation.

diff --git a/src/strip.c b/src/strip.c
index 1f7b3cabc0420c5930c7bc47a5338424d531fdea..fdebc5e2942cb17468f237fef2b7ad62a8ad99b3 100644 (file)
--- a/src/strip.c
+++ b/src/strip.c
@@ -792,9 +792,13 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
          if (shdr_info[shdr_info[cnt].group_idx].idx == 0)
            {
-             /* The section group section will be removed.  */
+             /* The section group section might be removed.
+                Don't remove the SHF_GROUP flag.  The section is
+                either also removed, in which case the flag doesn't matter.
+                Or it moves with the group into the debug file, then
+                it will be reconnected with the new group and should
+                still have the flag set.  */
              shdr_info[cnt].group_idx = 0;
-             shdr_info[cnt].shdr.sh_flags &= ~SHF_GROUP;
            }
        }
 
@@ -1368,7 +1372,9 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
                        && shdr_info[cnt].data->d_buf != NULL);
 
            Elf32_Word *grpref = (Elf32_Word *) shdr_info[cnt].data->d_buf;
-           for (size_t inner = 0;
+           /* First word is the section group flag.
+              Followed by section indexes, that need to be renumbered.  */
+           for (size_t inner = 1;
                 inner < shdr_info[cnt].data->d_size / sizeof (Elf32_Word);
                 ++inner)
              if (grpref[inner] < shnum)

diff --git a/src/unstrip.c b/src/unstrip.c
index e6f09478055e04186facad735f8cd5d129bb5dd7..03a0346246a808ba2f6f1e90a0c7701cccfd7854 100644 (file)
--- a/src/unstrip.c
+++ b/src/unstrip.c
@@ -696,6 +696,7 @@ struct section
 {
   Elf_Scn *scn;
   const char *name;
+  const char *sig;
   Elf_Scn *outscn;
   Dwelf_Strent *strent;
   GElf_Shdr shdr;
@@ -720,7 +721,8 @@ compare_alloc_sections (const struct section *s1, const struct section *s2,
 
 static int
 compare_unalloc_sections (const GElf_Shdr *shdr1, const GElf_Shdr *shdr2,
-                         const char *name1, const char *name2)
+                         const char *name1, const char *name2,
+                         const char *sig1, const char *sig2)
 {
   /* Sort by sh_flags as an arbitrary ordering.  */
   if (shdr1->sh_flags < shdr2->sh_flags)
@@ -734,6 +736,10 @@ compare_unalloc_sections (const GElf_Shdr *shdr1, const GElf_Shdr *shdr2,
   if (shdr1->sh_size > shdr2->sh_size)
     return 1;
 
+  /* Are they both SHT_GROUP sections? Then compare signatures.  */
+  if (sig1 != NULL && sig2 != NULL)
+    return strcmp (sig1, sig2);
+
   /* Sort by name as last resort.  */
   return strcmp (name1, name2);
 }
@@ -751,7 +757,8 @@ compare_sections (const void *a, const void *b, bool rel)
   return ((s1->shdr.sh_flags & SHF_ALLOC)
          ? compare_alloc_sections (s1, s2, rel)
          : compare_unalloc_sections (&s1->shdr, &s2->shdr,
-                                     s1->name, s2->name));
+                                     s1->name, s2->name,
+                                     s1->sig, s2->sig));
 }
 
 static int
@@ -986,6 +993,44 @@ get_section_name (size_t ndx, const GElf_Shdr *shdr, const Elf_Data *shstrtab)
   return shstrtab->d_buf + shdr->sh_name;
 }
 
+/* Returns the signature of a group section, or NULL if the given
+   section isn't a group.  */
+static const char *
+get_group_sig (Elf *elf, GElf_Shdr *shdr)
+{
+  if (shdr->sh_type != SHT_GROUP)
+    return NULL;
+
+  Elf_Scn *symscn = elf_getscn (elf, shdr->sh_link);
+  if (symscn == NULL)
+    error (EXIT_FAILURE, 0, _("bad sh_link for group section: %s"),
+          elf_errmsg (-1));
+
+  GElf_Shdr symshdr_mem;
+  GElf_Shdr *symshdr = gelf_getshdr (symscn, &symshdr_mem);
+  if (symshdr == NULL)
+    error (EXIT_FAILURE, 0, _("couldn't get shdr for group section: %s"),
+          elf_errmsg (-1));
+
+  Elf_Data *symdata = elf_getdata (symscn, NULL);
+  if (symdata == NULL)
+    error (EXIT_FAILURE, 0, _("bad data for group symbol section: %s"),
+          elf_errmsg (-1));
+
+  GElf_Sym sym_mem;
+  GElf_Sym *sym = gelf_getsym (symdata, shdr->sh_info, &sym_mem);
+  if (sym == NULL)
+    error (EXIT_FAILURE, 0, _("couldn't get symbol for group section: %s"),
+          elf_errmsg (-1));
+
+  const char *sig = elf_strptr (elf, symshdr->sh_link, sym->st_name);
+  if (sig == NULL)
+    error (EXIT_FAILURE, 0, _("bad symbol name for group section: %s"),
+          elf_errmsg (-1));
+
+  return sig;
+}
+
 /* Fix things up when prelink has moved some allocated sections around
    and the debuginfo file's section headers no longer match up.
    This fills in SECTIONS[0..NALLOC-1].outscn or exits.
@@ -1111,6 +1156,7 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
              sec->scn = elf_getscn (main, i + 1); /* Really just for ndx.  */
              sec->outscn = NULL;
              sec->strent = NULL;
+             sec->sig = get_group_sig (main, &sec->shdr);
              ++undo_nalloc;
            }
        }
@@ -1336,6 +1382,7 @@ more sections in stripped file than debug file -- arguments reversed?"));
       sections[i].scn = scn;
       sections[i].outscn = NULL;
       sections[i].strent = NULL;
+      sections[i].sig = get_group_sig (stripped, shdr);
     }
 
   const struct section *stripped_symtab = NULL;
@@ -1354,7 +1401,8 @@ more sections in stripped file than debug file -- arguments reversed?"));
 
   /* Locate a matching unallocated section in SECTIONS.  */
   inline struct section *find_unalloc_section (const GElf_Shdr *shdr,
-                                              const char *name)
+                                              const char *name,
+                                              const char *sig)
     {
       size_t l = nalloc, u = stripped_shnum - 1;
       while (l < u)
@@ -1362,7 +1410,8 @@ more sections in stripped file than debug file -- arguments reversed?"));
          size_t i = (l + u) / 2;
          struct section *sec = &sections[i];
          int cmp = compare_unalloc_sections (shdr, &sec->shdr,
-                                             name, sec->name);
+                                             name, sec->name,
+                                             sig, sec->sig);
          if (cmp < 0)
            u = i;
          else if (cmp > 0)
@@ -1435,7 +1484,8 @@ more sections in stripped file than debug file -- arguments reversed?"));
       else
        {
          /* Look for the section that matches.  */
-         sec = find_unalloc_section (shdr, name);
+         sec = find_unalloc_section (shdr, name,
+                                     get_group_sig (unstripped, shdr));
          if (sec == NULL)
            {
              /* An additional unallocated section is fine if not SHT_NOBITS.

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 04eeb4acdc3b34b53ad298942f59f703c107ebe3..c5f0d774c7b6ab5577c72cbb5ac0ed3a14eb4919 100644 (file)
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2018-09-12  Mark Wielaard  <mark@klomp.org>
+
+       * run-annobingroup.sh: Add x86_64 ET_REL testcase.
+       * testfile-annobingroup-x86_64.o.bz2: New test file.
+       * Makefile.am (EXTRA_DIST): Add testfile-annobingroup-x86_64.o.bz2.
+
 2018-09-18  Mark Wielaard  <mark@klomp.org>
 
        * backtrace-dwarf.c (thread_callback): Only error when

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 74c477e5fa276c8a22d06bf6a7ce5cadc2787070..15b429b792e7b99874fab86777ce0a738b460ce3 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -199,6 +199,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
             run-strip-nothing.sh run-strip-remove-keep.sh run-strip-g.sh \
             run-annobingroup.sh testfile-annobingroup.o.bz2 \
             testfile-annobingroup-i386.o.bz2 \
+            testfile-annobingroup-x86_64.o.bz2 \
             run-strip-strmerge.sh run-strip-nobitsalign.sh \
             testfile-nobitsalign.bz2 testfile-nobitsalign.strip.bz2 \
             run-strip-reloc.sh hello_i386.ko.bz2 hello_x86_64.ko.bz2 \

diff --git a/tests/run-annobingroup.sh b/tests/run-annobingroup.sh
index 700df321052321fe73f58d6591be28db51de2362..fd36e4acb25c9e057aad8d840a86df62738aeb4b 100755 (executable)
--- a/tests/run-annobingroup.sh
+++ b/tests/run-annobingroup.sh
@@ -120,4 +120,37 @@ EOF
 
 testrun ${abs_top_builddir}/src/elfcmp testfile-annobingroup-i386.o remerged.elf
 
+# echo "void * foo (void) { return foo; }" > testfile-annobingroup-x86_64.c
+# gcc -g -O2 -fplugin=annobin -c testfile-annobingroup-x86_64.c
+testfiles testfile-annobingroup-x86_64.o
+
+testrun_compare ${abs_top_builddir}/src/readelf -g testfile-annobingroup-x86_64.o << EOF
+
+Section group [ 1] '.group' with signature '.text.hot.group' contains 3 entries:
+  [11] .text.hot
+  [12] .gnu.build.attributes.hot
+  [13] .rela.gnu.build.attributes.hot
+
+Section group [ 2] '.group' with signature '.text.unlikely.group' contains 3 entries:
+  [14] .text.unlikely
+  [15] .gnu.build.attributes.unlikely
+  [16] .rela.gnu.build.attributes.unlikely
+
+Section group [ 3] '.group' with signature '.text.hot..group' contains 1 entry:
+  [26] .text.hot
+
+Section group [ 4] '.group' with signature '.text.unlikely..group' contains 1 entry:
+  [27] .text.unlikely
+EOF
+
+testrun ${abs_top_builddir}/src/strip -o stripped.elf -f debugfile.elf testfile-annobingroup-x86_64.o
+
+# This would/should work, except for the unknown NOTEs.
+# testrun ${abs_top_builddir}/src/elflint --gnu stripped.elf
+# testrun ${abs_top_builddir}/src/elflint --gnu --debug debugfile.elf
+
+testrun ${abs_top_builddir}/src/unstrip -o remerged.elf stripped.elf debugfile.elf
+
+testrun ${abs_top_builddir}/src/elfcmp testfile-annobingroup-x86_64.o remerged.elf
+
 exit 0

diff --git a/tests/testfile-annobingroup-x86_64.o.bz2 b/tests/testfile-annobingroup-x86_64.o.bz2
new file mode 100644 (file)
index 0000000..ec389db
Binary files /dev/null and b/tests/testfile-annobingroup-x86_64.o.bz2 differ