[PATCH] strip: Don't remove real symbols from allocated symbol tables.

[PATCH] strip: Don't remove real symbols from allocated symbol tables.

[Mark Wielaard]

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

Having a symbol in an allocated symbol table (like .dynsym) that
points to an unallocated section is wrong. Traditionally strip
has removed such symbols if they are section or group symbols.
But removing a real symbol from an allocate symbol table is hard
and probably a mistake. Really removing it means rewriting the
dynamic segment and hash sections. Since we don't do that, don't
remove the symbol (and corrupt the ELF file). Do warn and set
the symbol section to SHN_UNDEF.

https://bugzilla.redhat.com/show_bug.cgi?id=1380961

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 src/ChangeLog |  5 +++++
 src/strip.c   | 35 ++++++++++++++++++++++++-----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index e5b3b20..70d11f2 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2016-10-06  Mark Wielaard  <mjw@redhat.com>
+
+	* strip.c (handle_elf): Don't remove real symbols from allocated
+	symbol tables.
+
 2016-08-25  Mark Wielaard  <mjw@redhat.com>
 
 	* strip.c (handle_elf): Recompress with ELF_CHF_FORCE.
diff --git a/src/strip.c b/src/strip.c
index da093e9..819b67e 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1341,15 +1341,12 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
 		    /* Get the full section index, if necessary from the
 		       XINDEX table.  */
-		    if (sym->st_shndx != SHN_XINDEX)
-		      sec = shdr_info[sym->st_shndx].idx;
-		    else
-		      {
-			elf_assert (shndxdata != NULL
-				    && shndxdata->d_buf != NULL);
-
-			sec = shdr_info[xshndx].idx;
-		      }
+		    if (sym->st_shndx == SHN_XINDEX)
+		      elf_assert (shndxdata != NULL
+				  && shndxdata->d_buf != NULL);
+		    size_t sidx = (sym->st_shndx != SHN_XINDEX
+				   ? sym->st_shndx : xshndx);
+		    sec = shdr_info[sidx].idx;
 
 		    if (sec != 0)
 		      {
@@ -1387,6 +1384,24 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 			    shdr_info[cnt].shdr.sh_info = destidx - 1;
 			  }
 		      }
+		    else if ((shdr_info[cnt].shdr.sh_flags & SHF_ALLOC) != 0
+			     && GELF_ST_TYPE (sym->st_info) != STT_SECTION
+			     && shdr_info[sidx].shdr.sh_type != SHT_GROUP)
+		      {
+			/* Removing a real symbol from an allocated
+			   symbol table is hard and probably a
+			   mistake.  Really removing it means
+			   rewriting the dynamic segment and hash
+			   sections.  Just warn and set the symbol
+			   section to UNDEF.  */
+			error (0, 0,
+			       gettext ("Cannot remove symbol [%zd] from allocated symbol table [%zd]"), inner, cnt);
+			sym->st_shndx = SHN_UNDEF;
+			if (gelf_update_sym (shdr_info[cnt].data, destidx,
+					     sym) == 0)
+			  INTERNAL_ERROR (fname);
+			shdr_info[cnt].newsymidx[inner] = destidx++;
+		      }
 		    else if (debug_fname != NULL
 			     && shdr_info[cnt].debug_data == NULL)
 		      /* The symbol points to a section that is discarded
@@ -1394,8 +1409,6 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 			 this is a section or group signature symbol
 			 for a section which has been removed.  */
 		      {
-			size_t sidx = (sym->st_shndx != SHN_XINDEX
-					? sym->st_shndx : xshndx);
 			elf_assert (GELF_ST_TYPE (sym->st_info) == STT_SECTION
 				    || ((shdr_info[sidx].shdr.sh_type
 					 == SHT_GROUP)
-- 
1.8.3.1

Re: [PATCH] strip: Don't remove real symbols from allocated symbol tables.

[Mark Wielaard]

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

On Thu, 2016-10-06 at 11:53 -0700, Josh Stone wrote:
> On 10/06/2016 07:18 AM, Mark Wielaard wrote:
> > Having a symbol in an allocated symbol table (like .dynsym) that
> > points to an unallocated section is wrong. Traditionally strip
> > has removed such symbols if they are section or group symbols.
> > But removing a real symbol from an allocate symbol table is hard
> > and probably a mistake. Really removing it means rewriting the
> > dynamic segment and hash sections. Since we don't do that, don't
> > remove the symbol (and corrupt the ELF file). Do warn and set
> > the symbol section to SHN_UNDEF.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1380961
> > 
> > Signed-off-by: Mark Wielaard <mjw@redhat.com>
> 
> Works for me.  On the test from bugzilla, I get the expected warning:
> 
>   strip: Cannot remove symbol [1550] from allocated symbol table [3]
> 
> but ldd is perfectly happy with the result.
> 
> I also tried it with a manual "objcopy -R .rustc" beforehand, which I
> will probably do in the rpm to prevent that section from even being
> saved to debuginfo -- just discard it entirely.  Strip still works with
> the same warning and a working result.

Thanks for testing. It also has been in Fedora rawhide for some time
now. I pushed it to master.

Cheers,

Mark

Re: [PATCH] strip: Don't remove real symbols from allocated symbol tables.

[Josh Stone]

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

On 10/06/2016 07:18 AM, Mark Wielaard wrote:
> Having a symbol in an allocated symbol table (like .dynsym) that
> points to an unallocated section is wrong. Traditionally strip
> has removed such symbols if they are section or group symbols.
> But removing a real symbol from an allocate symbol table is hard
> and probably a mistake. Really removing it means rewriting the
> dynamic segment and hash sections. Since we don't do that, don't
> remove the symbol (and corrupt the ELF file). Do warn and set
> the symbol section to SHN_UNDEF.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1380961
> 
> Signed-off-by: Mark Wielaard <mjw@redhat.com>

Works for me.  On the test from bugzilla, I get the expected warning:

  strip: Cannot remove symbol [1550] from allocated symbol table [3]

but ldd is perfectly happy with the result.

I also tried it with a manual "objcopy -R .rustc" beforehand, which I
will probably do in the rpm to prevent that section from even being
saved to debuginfo -- just discard it entirely.  Strip still works with
the same warning and a working result.

> ---
>  src/ChangeLog |  5 +++++
>  src/strip.c   | 35 ++++++++++++++++++++++++-----------
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ChangeLog b/src/ChangeLog
> index e5b3b20..70d11f2 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-10-06  Mark Wielaard  <mjw@redhat.com>
> +
> +	* strip.c (handle_elf): Don't remove real symbols from allocated
> +	symbol tables.
> +
>  2016-08-25  Mark Wielaard  <mjw@redhat.com>
>  
>  	* strip.c (handle_elf): Recompress with ELF_CHF_FORCE.
> diff --git a/src/strip.c b/src/strip.c
> index da093e9..819b67e 100644
> --- a/src/strip.c
> +++ b/src/strip.c
> @@ -1341,15 +1341,12 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
>  
>  		    /* Get the full section index, if necessary from the
>  		       XINDEX table.  */
> -		    if (sym->st_shndx != SHN_XINDEX)
> -		      sec = shdr_info[sym->st_shndx].idx;
> -		    else
> -		      {
> -			elf_assert (shndxdata != NULL
> -				    && shndxdata->d_buf != NULL);
> -
> -			sec = shdr_info[xshndx].idx;
> -		      }
> +		    if (sym->st_shndx == SHN_XINDEX)
> +		      elf_assert (shndxdata != NULL
> +				  && shndxdata->d_buf != NULL);
> +		    size_t sidx = (sym->st_shndx != SHN_XINDEX
> +				   ? sym->st_shndx : xshndx);
> +		    sec = shdr_info[sidx].idx;
>  
>  		    if (sec != 0)
>  		      {
> @@ -1387,6 +1384,24 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
>  			    shdr_info[cnt].shdr.sh_info = destidx - 1;
>  			  }
>  		      }
> +		    else if ((shdr_info[cnt].shdr.sh_flags & SHF_ALLOC) != 0
> +			     && GELF_ST_TYPE (sym->st_info) != STT_SECTION
> +			     && shdr_info[sidx].shdr.sh_type != SHT_GROUP)
> +		      {
> +			/* Removing a real symbol from an allocated
> +			   symbol table is hard and probably a
> +			   mistake.  Really removing it means
> +			   rewriting the dynamic segment and hash
> +			   sections.  Just warn and set the symbol
> +			   section to UNDEF.  */
> +			error (0, 0,
> +			       gettext ("Cannot remove symbol [%zd] from allocated symbol table [%zd]"), inner, cnt);
> +			sym->st_shndx = SHN_UNDEF;
> +			if (gelf_update_sym (shdr_info[cnt].data, destidx,
> +					     sym) == 0)
> +			  INTERNAL_ERROR (fname);
> +			shdr_info[cnt].newsymidx[inner] = destidx++;
> +		      }
>  		    else if (debug_fname != NULL
>  			     && shdr_info[cnt].debug_data == NULL)
>  		      /* The symbol points to a section that is discarded
> @@ -1394,8 +1409,6 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
>  			 this is a section or group signature symbol
>  			 for a section which has been removed.  */
>  		      {
> -			size_t sidx = (sym->st_shndx != SHN_XINDEX
> -					? sym->st_shndx : xshndx);
>  			elf_assert (GELF_ST_TYPE (sym->st_info) == STT_SECTION
>  				    || ((shdr_info[sidx].shdr.sh_type
>  					 == SHT_GROUP)
>