[PATCH 1/2] unstrip: Don't try to use unstripped .symtab with stripped .strtab

[PATCH 1/2] unstrip: Don't try to use unstripped .symtab with stripped .strtab

[Kevin Cernekee]

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

Prematurely matching up the stripped and unstripped .strtab sections
in the "Match each debuginfo" loop can lead to a case where sec->outscn
gets populated for the stripped .strtab, which we normally want to
ignore.  This causes the .strtab override in the "Make sure each main
file section" loop to be skipped, so the code winds up using indices
from the unstripped .symtab to look up strings in the stripped .strtab.
This returns incorrect strings for a little while, and then fails
catastrophically when it tries to read past the end of the (smaller)
stripped file's .strtab section:

    eu-unstrip: invalid string offset in symbol [1589]

Fix this by adding logic to the "Match each debuginfo" loop to
treat the unstripped .strtab, .shstrtab, and .symtab sections
essentially the same way.

The new logic will break if the .strtab section shows up earlier than
the .symtab section.  We will assume this never happens in practice.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 src/ChangeLog |  7 ++++++-
 src/unstrip.c | 12 ++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 68baa2e7f6bf..ee67cffcdadd 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,4 +1,9 @@
-2015-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
+2016-10-22  Kevin Cernekee  <cernekee@chromium.org>
+
+	* unstrip.c: Fix "invalid string offset" error caused by using the
+	  unstripped .symtab with the stripped .strtab.
+
+2016-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
 
 	* arlib.c: Remove system.h include, add libeu.h include.
 	* arlib2.c: Remove sys/param.h include.
diff --git a/src/unstrip.c b/src/unstrip.c
index 3bf4b192b5db..cc3dcb26611a 100644
--- a/src/unstrip.c
+++ b/src/unstrip.c
@@ -1363,6 +1363,7 @@ more sections in stripped file than debug file -- arguments reversed?"));
   /* Match each debuginfo section with its corresponding stripped section.  */
   bool check_prelink = false;
   Elf_Scn *unstripped_symtab = NULL;
+  size_t unstripped_strndx = 0;
   size_t alloc_avail = 0;
   scn = NULL;
   while ((scn = elf_nextscn (unstripped, scn)) != NULL)
@@ -1374,11 +1375,12 @@ more sections in stripped file than debug file -- arguments reversed?"));
       if (shdr->sh_type == SHT_SYMTAB)
 	{
 	  unstripped_symtab = scn;
+	  unstripped_strndx = shdr->sh_link;
 	  continue;
 	}
 
       const size_t ndx = elf_ndxscn (scn);
-      if (ndx == unstripped_shstrndx)
+      if (ndx == unstripped_shstrndx || ndx == unstripped_strndx)
 	continue;
 
       const char *name = get_section_name (ndx, shdr, shstrtab);
@@ -1485,13 +1487,11 @@ more sections in stripped file than debug file -- arguments reversed?"));
 	    }
 
 	  if (unstripped_symtab != NULL && stripped_symtab != NULL
-	      && secndx == stripped_symtab->shdr.sh_link)
+	      && secndx == stripped_symtab->shdr.sh_link
+	      && unstripped_strndx != 0)
 	    {
 	      /* ... nor its string table.  */
-	      GElf_Shdr shdr_mem;
-	      GElf_Shdr *shdr = gelf_getshdr (unstripped_symtab, &shdr_mem);
-	      ELF_CHECK (shdr != NULL, _("cannot get section header: %s"));
-	      ndx_section[secndx - 1] = shdr->sh_link;
+	      ndx_section[secndx - 1] = unstripped_strndx;
 	      continue;
 	    }
 
-- 
2.8.3

Re: [PATCH 1/2] unstrip: Don't try to use unstripped .symtab with stripped .strtab

[Mark Wielaard]

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

On Sun, 2016-10-23 at 10:25 -0700, Kevin Cernekee wrote:
> Prematurely matching up the stripped and unstripped .strtab sections
> in the "Match each debuginfo" loop can lead to a case where sec->outscn
> gets populated for the stripped .strtab, which we normally want to
> ignore.  This causes the .strtab override in the "Make sure each main
> file section" loop to be skipped, so the code winds up using indices
> from the unstripped .symtab to look up strings in the stripped .strtab.
> This returns incorrect strings for a little while, and then fails
> catastrophically when it tries to read past the end of the (smaller)
> stripped file's .strtab section:
> 
>     eu-unstrip: invalid string offset in symbol [1589]
> 
> Fix this by adding logic to the "Match each debuginfo" loop to
> treat the unstripped .strtab, .shstrtab, and .symtab sections
> essentially the same way.

Looks good. Thanks for the analysis and the fix.
Applied to git master.

> The new logic will break if the .strtab section shows up earlier than
> the .symtab section.  We will assume this never happens in practice.

grin. This screams for someone to show up with such a file tomorrow :)
But in practice this indeed doesn't happen because all tools create the
sections in order. If someone does create such a file with the sections
reversed we'll have to add another pass over the section headers.

> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>

Thanks,

Mark

[PATCH 1/2] unstrip: Don't try to use unstripped .symtab with stripped .strtab

[Kevin Cernekee]

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

Prematurely matching up the stripped and unstripped .strtab sections
in the "Match each debuginfo" loop can lead to a case where sec->outscn
gets populated for the stripped .strtab, which we normally want to
ignore.  This causes the .strtab override in the "Make sure each main
file section" loop to be skipped, so the code winds up using indices
from the unstripped .symtab to look up strings in the stripped .strtab.
This returns incorrect strings for a little while, and then fails
catastrophically when it tries to read past the end of the (smaller)
stripped file's .strtab section:

    eu-unstrip: invalid string offset in symbol [1589]

Fix this by adding logic to the "Match each debuginfo" loop to
treat the unstripped .strtab, .shstrtab, and .symtab sections
essentially the same way.

The new logic will break if the .strtab section shows up earlier than
the .symtab section.  We will assume this never happens in practice.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 src/ChangeLog |  7 ++++++-
 src/unstrip.c | 12 ++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 68baa2e7f6bf..ee67cffcdadd 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,4 +1,9 @@
-2015-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
+2016-10-22  Kevin Cernekee  <cernekee@chromium.org>
+
+	* unstrip.c: Fix "invalid string offset" error caused by using the
+	  unstripped .symtab with the stripped .strtab.
+
+2016-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
 
 	* arlib.c: Remove system.h include, add libeu.h include.
 	* arlib2.c: Remove sys/param.h include.
diff --git a/src/unstrip.c b/src/unstrip.c
index 3bf4b192b5db..cc3dcb26611a 100644
--- a/src/unstrip.c
+++ b/src/unstrip.c
@@ -1363,6 +1363,7 @@ more sections in stripped file than debug file -- arguments reversed?"));
   /* Match each debuginfo section with its corresponding stripped section.  */
   bool check_prelink = false;
   Elf_Scn *unstripped_symtab = NULL;
+  size_t unstripped_strndx = 0;
   size_t alloc_avail = 0;
   scn = NULL;
   while ((scn = elf_nextscn (unstripped, scn)) != NULL)
@@ -1374,11 +1375,12 @@ more sections in stripped file than debug file -- arguments reversed?"));
       if (shdr->sh_type == SHT_SYMTAB)
 	{
 	  unstripped_symtab = scn;
+	  unstripped_strndx = shdr->sh_link;
 	  continue;
 	}
 
       const size_t ndx = elf_ndxscn (scn);
-      if (ndx == unstripped_shstrndx)
+      if (ndx == unstripped_shstrndx || ndx == unstripped_strndx)
 	continue;
 
       const char *name = get_section_name (ndx, shdr, shstrtab);
@@ -1485,13 +1487,11 @@ more sections in stripped file than debug file -- arguments reversed?"));
 	    }
 
 	  if (unstripped_symtab != NULL && stripped_symtab != NULL
-	      && secndx == stripped_symtab->shdr.sh_link)
+	      && secndx == stripped_symtab->shdr.sh_link
+	      && unstripped_strndx != 0)
 	    {
 	      /* ... nor its string table.  */
-	      GElf_Shdr shdr_mem;
-	      GElf_Shdr *shdr = gelf_getshdr (unstripped_symtab, &shdr_mem);
-	      ELF_CHECK (shdr != NULL, _("cannot get section header: %s"));
-	      ndx_section[secndx - 1] = shdr->sh_link;
+	      ndx_section[secndx - 1] = unstripped_strndx;
 	      continue;
 	    }
 
-- 
2.8.3