[PATCH] D48952: [LLD] [COFF] Disallow replacing DefinedImport* symbols with other symbols

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo created this revision.
mstorsjo added reviewers: ruiu, pcc.

When DefinedImportThunk and DefinedImportData symbols are created, a pointer to them is stored in the ImportFile class, stored as a pointer to the specific subclass. If these symbols are replaced, these pointers no longer point to this particular base class.

This fixes crashes that were possible before, by touching these symbols that now are populated as e.g. a DefinedRegular, via the old pointers with DefinedImportThunk type.

This is one way of solving the issue, the alternative would be to not store pointers to the subclass but actually check them on use.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48952

Files:
  COFF/SymbolTable.cpp
  test/COFF/Inputs/otherFunc.s
  test/COFF/thunk-replace.s


Index: test/COFF/thunk-replace.s
===================================================================
--- /dev/null
+++ test/COFF/thunk-replace.s
@@ -0,0 +1,15 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-win32 %s -filetype=obj -o %t.main.obj
+# RUN: llvm-mc -triple=x86_64-win32 %p/Inputs/otherFunc.s -filetype=obj -o %t.other.obj
+# RUN: llvm-ar rcs %t.other.lib %t.other.obj
+# RUN: not lld-link -out:%t.exe -entry:main %t.main.obj %p/Inputs/std64.lib %t.other.lib -opt:noref 2>&1 | FileCheck %s
+# CHECK: duplicate symbol: MessageBoxA
+
+.global main
+.text
+main:
+  callq MessageBoxA
+  callq ExitProcess
+  callq otherFunc
+  ret
Index: test/COFF/Inputs/otherFunc.s
===================================================================
--- /dev/null
+++ test/COFF/Inputs/otherFunc.s
@@ -0,0 +1,7 @@
+.global otherFunc
+.global MessageBoxA
+.text
+otherFunc:
+  ret
+MessageBoxA:
+  ret
Index: COFF/SymbolTable.cpp
===================================================================
--- COFF/SymbolTable.cpp
+++ COFF/SymbolTable.cpp
@@ -301,7 +301,8 @@
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedRegular>(S))
+  if (WasInserted || (!isa<DefinedRegular>(S) && !isa<DefinedImportThunk>(S) &&
+                      !isa<DefinedImportData>(S)))
     replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ false,
                                   /*IsExternal*/ true, Sym, C);
   else
@@ -317,7 +318,8 @@
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedRegular>(S)) {
+  if (WasInserted || (!isa<DefinedRegular>(S) && !isa<DefinedImportThunk>(S) &&
+                      !isa<DefinedImportData>(S))) {
     replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ true,
                                   /*IsExternal*/ true, Sym, nullptr);
     return {S, true};
@@ -334,7 +336,9 @@
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedCOFF>(S))
+  if (!WasInserted && (isa<DefinedImportThunk>(S) || isa<DefinedImportData>(S)))
+    reportDuplicate(S, F);
+  else if (WasInserted || !isa<DefinedCOFF>(S))
     replaceSymbol<DefinedCommon>(S, F, N, Size, Sym, C);
   else if (auto *DC = dyn_cast<DefinedCommon>(S))
     if (Size > DC->getSize())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48952.154150.patch
Type: text/x-patch
Size: 2420 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180704/9fad4c11/attachment.bin>