Commit: PR 22875: Stop strip corrupting unknown relocs

[Maciej W. Rozycki]

On Sun, 4 Mar 2018, Alan Modra wrote:

> >  Nevertheless it has caused failures with the new test case and the 
> > `mips64-openbsd' and `mips64el-openbsd' targets:
> > 
> > Executing on host: .../binutils/strip-new  -g tmpdir/bintest.o -o tmpdir/copy.o   (timeout = 300)
> > .../binutils/strip-new: tmpdir/bintest.o: unsupported relocation type 0xf1
> > .../binutils/strip-new: tmpdir/bintest.o: unsupported relocation type 0xf1
> > 
> > failed with: <.../binutils/strip-new: tmpdir/bintest.o: unsupported relocation type 0xf1>, expected: <.* bad value>
> > .../binutils/strip-new: tmpdir/bintest.o: unsupported relocation type 0xf1
> > FAIL: binutils-all/strip-13
> > 
> > which both default to the n64 rather than o32 or n32 MIPS ABI.
> > 
> >  Offhand the error message appears correct to me, and arguably more useful 
> > than ambiguous and rather cryptic "bad value".
> 
> You're supposed to get the "unsupported relocation" error as well as
> the "bad value" error.  The reason why that isn't the case for mips64
> is that the error status isn't propagated up the call chain.

 Hmm, the o32 MIPS target backend however does not issue the "unsupported 
relocation" error, and neither does the i386 target AFAICS.  It looks to 
me like the reason is these targets do not support the RELA relocation 
format, as further indicated by `readelf -r':

readelf: Error: Section 4 has invalid sh_entsize of 0000000000000000
readelf: Error: (Using the expected size of 12 for the rest of this dump)

and reject broken input outright, before even getting to relocation 
processing.  I think it is a bug in the test case, which should use the 
REL format for backends which require it (we could possibly check both for 
dual-format backends).  And then I think the test case should get updated 
to expect both errors, as now we get false positives for o32 MIPS and i386 
objects.  Better error reporting for the unsupported relocation format 
would be good having too.

> Something like the following ought to fix that.
> 
> diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
> index ea2e921..9501613 100644
> --- a/bfd/elf64-mips.c
> +++ b/bfd/elf64-mips.c
> @@ -3611,7 +3611,7 @@ mips_elf64_rtype_to_howto (bfd *abfd, unsigned int r_type, bfd_boolean rela_p)
>  	  _bfd_error_handler (_("%pB: unsupported relocation type %#x"),
>  			      abfd, r_type);
>  	  bfd_set_error (bfd_error_bad_value);
> -	  r_type = R_MIPS_NONE;
> +	  return NULL;
>  	}
>        if (rela_p)
>  	return &mips_elf64_howto_table_rela[r_type];
> @@ -3800,6 +3800,8 @@ mips_elf64_slurp_one_reloc_table (bfd *abfd, asection *asect,
>  	  relent->addend = rela.r_addend;
>  
>  	  relent->howto = mips_elf64_rtype_to_howto (abfd, type, rela_p);
> +	  if (relent->howto == NULL)
> +	    goto error_return;
>  
>  	  ++relent;
>  	}

 Thanks for fixing it.

> >  However I find it strange 
> > that the MIPS n64 ABI backend is the only one to give this different error 
> > message, despite that backends for the three MIPS ABIs are all structured 
> > similarly.
> 
> An even more strange fact is that mips64el-openbsd creates really odd
> relocs for the strip-13 testcase.  There seems to be a word swap..
> 
> Relocation section '.rela.text' at offset 0x90 contains 2 entries:
>     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
> 0000000000000000  000000f100000000 R_MIPS_NONE           readelf: Error:  bad symbol index: 000000f1 in reloc
>                     Type2: R_MIPS_NONE      
>                     Type3: R_MIPS_NONE      
> 0000000000000000  0000000000000000 R_MIPS_NONE                               0
>                     Type2: R_MIPS_NONE      
>                     Type3: R_MIPS_NONE      

 Well, that's a consequence of how n64 MIPS psABI encodes relocation 
triplets:

typedef struct
  {
    Elf64_Addr r_offset;
    Elf64_Word r_sym;
    Elf64_Byte r_ssym;
    Elf64_Byte r_type3;
    Elf64_Byte r_type2;
    Elf64_Byte r_type;
    Elf64_Sxword r_addend;
  }
Elf64_Rela;

Again, I think it's up to the test case to get it right when encoding 
relocations manually.

> Oh, and the bad symbol index gets you a segfault on trying to run
> strip -g, at this line
>       if ((*ptr->sym_ptr_ptr)->the_bfd->xvec != abfd->xvec
> in elf64-mip.c:mips_elf64_write_rela.  the_bfd is NULL.

 Will look into it (including your observation downthread).

  Maciej