[PATCH] D52815: [ELF] Fix crash on invalid undefined local symbols

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai created this revision.
smeenai added reviewers: grimar, ruiu.
Herald added subscribers: arichardson, emaste.
Herald added a reviewer: espindola.

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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D52815

Files:
  ELF/InputFiles.cpp
  test/ELF/Inputs/undefined-local-symbol-in-dso.so
  test/ELF/invalid-undefined-local-symbol-in-dso.s


Index: test/ELF/invalid-undefined-local-symbol-in-dso.s
===================================================================
--- /dev/null
+++ test/ELF/invalid-undefined-local-symbol-in-dso.s
@@ -0,0 +1,6 @@
+# REQUIRES: x86
+
+# We used to crash on this
+# 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 'i' in global part of symbol table in file {{.*}}undefined-local-symbol-in-dso.so
Index: ELF/InputFiles.cpp
===================================================================
--- ELF/InputFiles.cpp
+++ ELF/InputFiles.cpp
@@ -994,25 +994,25 @@
   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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D52815.168057.patch
Type: text/x-patch
Size: 2130 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181003/c2fcd227/attachment.bin>