strip: Ignore --reloc-debug-sections-only for non-ET_REL files.

Message ID 20241105225338.1279732-1-amerey@redhat.com
State Committed
Headers
Series strip: Ignore --reloc-debug-sections-only for non-ET_REL files. |

Commit Message

Aaron Merey Nov. 5, 2024, 10:53 p.m. UTC
  strip --reloc-debug-sections-only is expected to be a no-op for
non-ET_REL files.  This was not enforced in the code.  Sections
were copied over to a new output file and normally its contents
would be identical to the input file.

However the output file is not identical to a non-ET_REL input
file if the linker organized sections such that the indices of
SHF_ALLOC sections are not in a contigous group.

In this case strip will modify sections in order to keep all SHF_ALLOC
sections in a contiguous group.

Fix this by ignoring --reloc-debug-sections-only for non-ET_REL files.

https://sourceware.org/bugzilla/show_bug.cgi?id=32253

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 src/strip.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

Comments

Mark Wielaard Nov. 6, 2024, 7:35 p.m. UTC | #1
Hi Aaron,

On Tue, Nov 05, 2024 at 05:53:38PM -0500, Aaron Merey wrote:
> strip --reloc-debug-sections-only is expected to be a no-op for
> non-ET_REL files.  This was not enforced in the code.  Sections
> were copied over to a new output file and normally its contents
> would be identical to the input file.
> 
> However the output file is not identical to a non-ET_REL input
> file if the linker organized sections such that the indices of
> SHF_ALLOC sections are not in a contigous group.
> 
> In this case strip will modify sections in order to keep all SHF_ALLOC
> sections in a contiguous group.
> 
> Fix this by ignoring --reloc-debug-sections-only for non-ET_REL files.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=32253
> 
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  src/strip.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/strip.c b/src/strip.c
> index 403e0f6f..3812fb17 100644
> --- a/src/strip.c
> +++ b/src/strip.c
> @@ -1139,6 +1139,13 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
>  
>    if (reloc_debug_only)
>      {
> +      if (ehdr->e_type != ET_REL)
> +	{
> +	  /* Only ET_REL files should have debug relocations to remove.  */
> +	  error (0, 0, _("Ignoring --reloc-debug-sections-only for " \
> +			 "non-ET_REL file '%s'"), fname);
> +	  goto fail_close;
> +	}

Do we have to fail here?  I think it is nicer for the user to just
turn this into an warning with

if (ehdr->e_type != ET_REL) else if (handle_debug_relocs (...))

>        if (handle_debug_relocs (elf, ebl, newelf, ehdr, fname, shstrndx,
>  			       &lastsec_offset, &lastsec_size) != 0)
>  	{

Cheers,

Mark
  
Aaron Merey Nov. 6, 2024, 9:15 p.m. UTC | #2
Hi Mark,

On Wed, Nov 6, 2024 at 2:35 PM Mark Wielaard <mark@klomp.org> wrote:
> >    if (reloc_debug_only)
> >      {
> > +      if (ehdr->e_type != ET_REL)
> > +     {
> > +       /* Only ET_REL files should have debug relocations to remove.  */
> > +       error (0, 0, _("Ignoring --reloc-debug-sections-only for " \
> > +                      "non-ET_REL file '%s'"), fname);
> > +       goto fail_close;
> > +     }
>
> Do we have to fail here?  I think it is nicer for the user to just
> turn this into an warning with
>
> if (ehdr->e_type != ET_REL) else if (handle_debug_relocs (...))

If we don't fail then strip will need to do more work copying sections
over to the 'newelf' handle in order to properly overwrite the input file
with newelf.

Since --reloc-debug-sections-only should be a no-op for non-ET_REL files
it makes sense to skip that extra work and avoid overwriting the input file.

Aaron
  
Mark Wielaard Nov. 6, 2024, 11:32 p.m. UTC | #3
Hi Aaron,

On Wed, Nov 06, 2024 at 04:15:30PM -0500, Aaron Merey wrote:
> On Wed, Nov 6, 2024 at 2:35 PM Mark Wielaard <mark@klomp.org> wrote:
> > >    if (reloc_debug_only)
> > >      {
> > > +      if (ehdr->e_type != ET_REL)
> > > +     {
> > > +       /* Only ET_REL files should have debug relocations to remove.  */
> > > +       error (0, 0, _("Ignoring --reloc-debug-sections-only for " \
> > > +                      "non-ET_REL file '%s'"), fname);
> > > +       goto fail_close;
> > > +     }
> >
> > Do we have to fail here?  I think it is nicer for the user to just
> > turn this into an warning with
> >
> > if (ehdr->e_type != ET_REL) else if (handle_debug_relocs (...))
> 
> If we don't fail then strip will need to do more work copying sections
> over to the 'newelf' handle in order to properly overwrite the input file
> with newelf.
> 
> Since --reloc-debug-sections-only should be a no-op for non-ET_REL files
> it makes sense to skip that extra work and avoid overwriting the input file.

Aha, so by not goto done you skip the actual (supposed to do nothing)
work. goto fail_close isn't so much "failing", but more about closing
without doing any more work. Which is fine because when
--reloc-debug-sections-only "No other stripping is performed".

OK. Sorry for being dense.

Cheers,

Mark
  
Aaron Merey Nov. 7, 2024, 2:47 p.m. UTC | #4
On Wed, Nov 6, 2024 at 6:32 PM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Aaron,
>
> On Wed, Nov 06, 2024 at 04:15:30PM -0500, Aaron Merey wrote:
> > On Wed, Nov 6, 2024 at 2:35 PM Mark Wielaard <mark@klomp.org> wrote:
> > > >    if (reloc_debug_only)
> > > >      {
> > > > +      if (ehdr->e_type != ET_REL)
> > > > +     {
> > > > +       /* Only ET_REL files should have debug relocations to remove.  */
> > > > +       error (0, 0, _("Ignoring --reloc-debug-sections-only for " \
> > > > +                      "non-ET_REL file '%s'"), fname);
> > > > +       goto fail_close;
> > > > +     }
> > >
> > > Do we have to fail here?  I think it is nicer for the user to just
> > > turn this into an warning with
> > >
> > > if (ehdr->e_type != ET_REL) else if (handle_debug_relocs (...))
> >
> > If we don't fail then strip will need to do more work copying sections
> > over to the 'newelf' handle in order to properly overwrite the input file
> > with newelf.
> >
> > Since --reloc-debug-sections-only should be a no-op for non-ET_REL files
> > it makes sense to skip that extra work and avoid overwriting the input file.
>
> Aha, so by not goto done you skip the actual (supposed to do nothing)
> work. goto fail_close isn't so much "failing", but more about closing
> without doing any more work. Which is fine because when
> --reloc-debug-sections-only "No other stripping is performed".
>
> OK. Sorry for being dense.

Thanks Mark, pushed.

Aaron
  

Patch

diff --git a/src/strip.c b/src/strip.c
index 403e0f6f..3812fb17 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1139,6 +1139,13 @@  handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
   if (reloc_debug_only)
     {
+      if (ehdr->e_type != ET_REL)
+	{
+	  /* Only ET_REL files should have debug relocations to remove.  */
+	  error (0, 0, _("Ignoring --reloc-debug-sections-only for " \
+			 "non-ET_REL file '%s'"), fname);
+	  goto fail_close;
+	}
       if (handle_debug_relocs (elf, ebl, newelf, ehdr, fname, shstrndx,
 			       &lastsec_offset, &lastsec_size) != 0)
 	{