[ELF] Fix crash on invalid undefined local symbols

[smeenai]

Summary

r320770 made LLD handle invalid DSOs where local symbols were found in
the global part of the symbol table. Unfortunately, it didn't handle the
case where those local symbols were also undefined, and r326242 exposed
an assertion failure in that case. Just warn on that case instead of
crashing, by moving the local binding check before the undefined symbol
addition.

The input file for the test is crafted by hand, since I don't know of
any tool that would produce such a broken DSO. I also don't understand
what it even means for a symbol to be undefined but have STB_LOCAL
binding - I don't think that combination makes any sense - but we have
found broken DSOs of this nature that we were linking against.

[grimar]

We have a subfolder test\ELF\invalid for the tests that have invalid inputs. I think the test case should be placed there.

test/ELF/invalid-undefined-local-symbol-in-dso.s

3 ↗

(On Diff #168057)

Please expand the comment to describe what is exactly wrong with the object.

4 ↗

(On Diff #168057)

Can you use yaml for the test case?

You perhaps can use obj2yaml on the file you have.

[smeenai]

Address review comments

test/ELF/invalid-undefined-local-symbol-in-dso.s

4 ↗

(On Diff #168057)

I tried using obj2yaml, but it's unable to produce a broken DSO of this nature. I need to have a local symbol in the global section of the dynamic symbol table, i.e. have a local symbol with index >= the dynamic symbol table section's sh_info. I don't think obj2yaml is able to produce an output file like that. I added super detailed instructions for creating the broken DSO instead.

[ruiu]

LGTM

I agree with you that it is not easy to create a broken object file. In this case, checking in a binary is fine.

[smeenai]

test/ELF/invalid/undefined-local-symbol-in-dso.test

64 ↗

(On Diff #168204)

Whoops, this is a leftover line. I'll remove before committing.

lld/trunk/ELF/InputFiles.cpp

template <class ELFT> void SharedFile<ELFT>::parseRest() {
   // lot of allocations.
   SmallString<0> VersionedNameBuffer;
 
   // Add symbols to the symbol table.
   ArrayRef<Elf_Sym> Syms = this->getGlobalELFSyms();
   for (size_t I = 0; I < Syms.size(); ++I) {
     const Elf_Sym &Sym = Syms[I];
 
-    StringRef Name = CHECK(Sym.getName(this->StringTable), this);
-    if (Sym.isUndefined()) {
-      Symbol *S = Symtab->addUndefined<ELFT>(Name, Sym.getBinding(),
-                                             Sym.st_other, Sym.getType(),
-                                             /*CanOmitFromDynSym=*/false, this);
-      S->ExportDynamic = true;
-      continue;
-    }
-
     // ELF spec requires that all local symbols precede weak or global
     // symbols in each symbol table, and the index of first non-local symbol
     // is stored to sh_info. If a local symbol appears after some non-local
     // symbol, that's a violation of the spec.
+    StringRef Name = CHECK(Sym.getName(this->StringTable), this);
     if (Sym.getBinding() == STB_LOCAL) {
       warn("found local symbol '" + Name +
            "' in global part of symbol table in file " + toString(this));
       continue;
     }
 
+    if (Sym.isUndefined()) {
+      Symbol *S = Symtab->addUndefined<ELFT>(Name, Sym.getBinding(),
+                                             Sym.st_other, Sym.getType(),
+                                             /*CanOmitFromDynSym=*/false, this);
+      S->ExportDynamic = true;
+      continue;
+    }
+
     // MIPS BFD linker puts _gp_disp symbol into DSO files and incorrectly
     // assigns VER_NDX_LOCAL to this section global symbol. Here is a
     // workaround for this bug.
     uint32_t Idx = Versyms[I] & ~VERSYM_HIDDEN;
     if (Config->EMachine == EM_MIPS && Idx == VER_NDX_LOCAL &&
         Name == "_gp_disp")
       continue;
 

lld/trunk/test/ELF/invalid/undefined-local-symbol-in-dso.test

+# REQUIRES: x86
+
+# LLD used to crash when linking against a DSO with an undefined STB_LOCAL
+# symbol in the global part of the dynamic symbol table (i.e. an STB_LOCAL
+# symbol with an index >= the sh_info of the dynamic symbol table section). Such
+# a DSO is very broken, because local symbols should precede all global symbols
+# in the symbol table, and because having a symbol that's both undefined and
+# STB_LOCAL is a nonsensical combination. Nevertheless, we should warn on such
+# input files instead of crashing.
+
+# We've found actual broken DSOs of this sort in the wild, but for this test, we
+# created a reduced broken input file. There are no tools capable of producing a
+# broken DSO of this nature, so instead we created a valid DSO with an undefiend
+# global symbol in the dynamic symbol table and then manually edited the binary
+# to make that symbol local. The valid DSO was created as follows:
+
+```
+% cat undef.s
+.hidden bar
+bar:
+  movq  foo@GOT, %rax
+
+% llvm-mc -triple=x86_64-linux-gnu -filetype=obj -o undef.o undef.s
+% ld.lld --no-rosegment -shared -o undefined-local-symbol-in-dso.so undef.o
+% strip undef.so
+```
+
+# (--no-rosegment and stripping are unnecessary; they just produce a smaller
+# binary)
+
+# This DSO should only have a single dynamic symbol table entry for foo, and
+# then we can use a small C program to modify that symbol table entry to be
+# STB_LOCAL instead of STB_GLOBAL.
+
+```
+#include <elf.h>
+#include <stdio.h>
+
+int main(int argc, char *argv[]) {
+  FILE *F = fopen(argv[1], "rb+");
+
+  Elf64_Ehdr Ehdr;
+  fread(&Ehdr, sizeof(Ehdr), 1, F);
+  fseek(F, Ehdr.e_shoff, SEEK_SET);
+
+  Elf64_Shdr Shdr;
+  do {
+    fread(&Shdr, sizeof(Shdr), 1, F);
+  } while (Shdr.sh_type != SHT_DYNSYM);
+
+  Elf64_Sym Sym;
+  fseek(F, Shdr.sh_offset + sizeof(Elf64_Sym), SEEK_SET);
+  fread(&Sym, sizeof(Sym), 1, F);
+  Sym.st_info = STB_LOCAL << 4 | ELF64_ST_TYPE(Sym.st_info);
+  fseek(F, Shdr.sh_offset + sizeof(Elf64_Sym), SEEK_SET);
+  fwrite(&Sym, sizeof(Sym), 1, F);
+  fclose(F);
+}
+```
+
+# (the C program just takes its input DSO and modifies the binding of the first
+# dynamic symbol table entry to be STB_LOCAL instead of STB_GLOBAL)
+
+# RUN: ld.lld %p/Inputs/undefined-local-symbol-in-dso.so -o %t 2>&1 | \
+# RUN:   FileCheck -check-prefix=WARN %s
+# WARN: found local symbol 'foo' in global part of symbol table in file {{.*}}undefined-local-symbol-in-dso.so