[PATCH] D25365: [ELF] - Do not crash on invalid local symbol.

[Rafael Espíndola via llvm-commits]

I am building a debug version to take a look, but how is this case
different from a local absolute? I.E., what code path prevents us from
crashing on those?

Cheers,
Rafael


On 7 October 2016 at 08:28, George Rimar <grimar at accesssoftek.com> wrote:
> grimar created this revision.
> grimar added reviewers: ruiu, rafael, davide.
> grimar added subscribers: llvm-commits, grimar, evgeny777.
>
> I had a bunch of crashes during last AFL runs.
>
> Problem is next. Object contains local symbol of type STT_NOTYPE
> (it just should not be STT_FILE or STT_SECTION to crash).
>
> Has section index greater than SHN_LORESERVE, so next code returns 0
> template <class ELFT>
>
>   uint32_t ELFFileBase<ELFT>::getSectionIndex(const Elf_Sym &Sym) const {
>   ...
>     if (I >= ELF::SHN_LORESERVE)
>       return 0;
>     return I;
>   }
>
> Then DefinedRegular is created:
>
>     if (Sym->st_shndx == SHN_UNDEF)
>       return new (this->Alloc)
>           Undefined(Sym->st_name, Sym->st_other, Sym->getType(), this);
>     return new (this->Alloc) DefinedRegular<ELFT>(*Sym, Sec);
>   }
>
> And finally code is crashes in shouldKeepInSymtab() because Sec is null there.
>
> Patch fixes that. Since it is a crash issue and we did not had reports about that, I think fix is fine.
>
>
> https://reviews.llvm.org/D25365
>
> Files:
>   ELF/Writer.cpp
>   test/ELF/invalid/Inputs/local-symbols.elf
>   test/ELF/invalid/local-symbols.s
>
>
> Index: test/ELF/invalid/local-symbols.s
> ===================================================================
> --- test/ELF/invalid/local-symbols.s
> +++ test/ELF/invalid/local-symbols.s
> @@ -0,0 +1,3 @@
> +## local-symbols.elf has invalid local symbol.
> +# RUN: not ld.lld %p/Inputs/local-symbols.elf -o %t2 2>&1 | FileCheck %s
> +# CHECK: object contains invalid symbols
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -308,7 +308,8 @@
>  }
>
>  template <class ELFT>
> -static bool shouldKeepInSymtab(InputSectionBase<ELFT> *Sec, StringRef SymName,
> +static bool shouldKeepInSymtab(elf::ObjectFile<ELFT> *F,
> +                               InputSectionBase<ELFT> *Sec, StringRef SymName,
>                                 const SymbolBody &B) {
>    if (B.isFile())
>      return false;
> @@ -335,6 +336,9 @@
>    if (Config->Discard == DiscardPolicy::Locals)
>      return false;
>
> +  if (!Sec)
> +    fatal(getFilename(F) + ": object contains invalid symbols");
> +
>    return !(Sec->getSectionHdr()->sh_flags & SHF_MERGE);
>  }
>
> @@ -374,7 +378,7 @@
>          fatal(getFilename(F) + ": invalid symbol name offset");
>        StringRef SymName(StrTab.data() + B->getNameOffset());
>        InputSectionBase<ELFT> *Sec = DR->Section;
> -      if (!shouldKeepInSymtab<ELFT>(Sec, SymName, *B))
> +      if (!shouldKeepInSymtab<ELFT>(F, Sec, SymName, *B))
>          continue;
>        ++Out<ELFT>::SymTab->NumLocals;
>        if (Config->Relocatable)
>
>