[ELF] - Fixed crash on invalid input.

[grimar]

Summary

I took the input from https://llvm.org/bugs/show_bug.cgi?id=30540, it was
"id_000000,sig_11,src_000000,op_flip1,pos_98"

File contains invalid symbol name offset (too large) and lld just crashes,
patch fixes the issue.

I wonder may be we want a separate folder for invalid input tests, like we have
for linkerscript ?

[davide]

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

[grimar]

In D24970#554040, @davide wrote:

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

It is just 480 bytes, does it worth that you think ?

[davide]

In D24970#554045, @grimar wrote:

In D24970#554040, @davide wrote:

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

It is just 480 bytes, does it worth that you think ?

If the test can be reduced, I don't see a reason why we shouldn't do it.

[emaste]

In D24970#554040, @davide wrote:

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

Can we craft a test to exercise the invalid symbol name offset error instead?

[grimar]

In D24970#554047, @davide wrote:

In D24970#554045, @grimar wrote:

In D24970#554040, @davide wrote:

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

It is just 480 bytes, does it worth that you think ?

If the test can be reduced, I don't see a reason why we shouldn't do it.

We have about 20 binaries already in input folder:
*.elf, *.o and *.so files, I think it is just hex-edited files.

I`ll try to craft test output using yaml2obj tomorrow for this.

[grimar]

In D24970#554047, @davide wrote:

In D24970#554045, @grimar wrote:

In D24970#554040, @davide wrote:

I think the tests should live in a different directory, yes.
Also, you might want to try reducing this binary.

It is just 480 bytes, does it worth that you think ?

If the test can be reduced, I don't see a reason why we shouldn't do it.

So I do not know way how to craft the testcase for this without hex editor.
yaml2obj does not seem to have functionality to set Symbol st_name
which is offset in symtab, what is used in that patch. It has next code to set up
st_name:

Symbol.st_name = DotStrtab.getOffset(Sym.Name);

where DotStrtab is just a StringTableBuilder, so there is no way to set up huge offset manually from yaml.

Also it seems not possible to create own .symtab, when I put into yaml the following description:

Sections:
  - Name:            .strtab
    Type:            SHT_STRTAB
    Content:         00
    Size:            2

I have next error: "Repeated section name: '.strtab' at YAML section number 0.", because tool tries to create it own
.symtab.

Taking in account that new binary input is among the smallest ones from what we already have in lld and the fact that crafting
such special object is not possible using existent tool functionality, I suggest to leave it as is.

[emaste]

Taking in account that new binary input is among the smallest ones from what we already have in lld and the fact that crafting
such special object is not possible using existent tool functionality, I suggest to leave it as is.

OK, sounds fine to me. We can always replace it with yaml2obj or something else later on if it grows the ability to create broken output objects.

[ruiu]

LGTM

ELF/Writer.cpp

391 ↗

(On Diff #72670)

Please include a filename.

[• rafael]

LGTM too. Thanks!

lld/trunk/ELF/Writer.cpp

 }
 
 // Local symbols are not in the linker's symbol table. This function scans
 // each object file's symbol table to copy local symbols to the output.
 template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
   if (!Out<ELFT>::SymTab)
     return;
   for (elf::ObjectFile<ELFT> *F : Symtab<ELFT>::X->getObjectFiles()) {
-    const char *StrTab = F->getStringTable().data();
+    StringRef StrTab = F->getStringTable();
     for (SymbolBody *B : F->getLocalSymbols()) {
       auto *DR = dyn_cast<DefinedRegular<ELFT>>(B);
       // No reason to keep local undefined symbol in symtab.
       if (!DR)
         continue;
       if (!includeInSymtab<ELFT>(*B))
         continue;
-      StringRef SymName(StrTab + B->getNameOffset());
+      if (B->getNameOffset() >= StrTab.size())
+        fatal(getFilename(F) + ": invalid symbol name offset");
+      StringRef SymName(StrTab.data() + B->getNameOffset());
       InputSectionBase<ELFT> *Sec = DR->Section;
       if (!shouldKeepInSymtab<ELFT>(Sec, SymName, *B))
         continue;
       ++Out<ELFT>::SymTab->NumLocals;
       if (Config->Relocatable)
         B->DynsymIndex = Out<ELFT>::SymTab->NumLocals;
       F->KeptLocalSyms.push_back(
           std::make_pair(DR, Out<ELFT>::SymTab->StrTabSec.addString(SymName)));

lld/trunk/test/ELF/invalid/symbol-name.s

+# REQUIRES: x86
+
+## symbol-name-offset.elf contains symbol with invalid (too large)
+## st_name value.
+# RUN: not ld.lld %S/Inputs/symbol-name-offset.elf \
+# RUN:   -o %t 2>&1 | FileCheck %s
+# CHECK: invalid symbol name offset