[PATCH] D69304: [llvm-objcopy] - Do not crash on object that has relocations but no symbol table.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:908
   for (const Relocation &Reloc : Relocations)
     Reloc.RelocSymbol->Referenced = true;
 }
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > This looks dodgy if no symbol exists.
> > "relocation references symbol with index..." error is triggered earlier than execution flow might reach here.
> > 
> > It is reported from `initRelocations` which is called inside `Reader.create()` here:
> > https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L773
> > 
> > `markSymbols()` is called from `updateAndRemoveSymbols` which is itself caled in `handleArgs`:
> > https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L799
> > https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L613
> > https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L402
> > 
> > `handleArgs` is called later than `Reader.create()`. 
> > 
> > Also, it checks for an empty symbol table at its begining:
> > https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L350
> I've found this concern was valid. To trigger an issue we need an object with a symbol table and a relocation with a symbol index of 0. Then it is possible to reach `markSymbol` for example when `--strip-unneeded` is given.
> 
> I've added a test to `no-symbol-relocation.test`.
It's probably safer this way, even if it was unreachable. But I'm glad my caution was not misplaced this time!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69304/new/

https://reviews.llvm.org/D69304