Fix direct calls to __wrap_sym when it is relocated

[matthew.koontz]

Summary

Before, direct calls to __wrap_sym would not map to valid PLT entries, so they would crash at runtime. This change maps such calls to the same PLT entry as calls to sym that are then wrapped.

[ruiu]

ELF/SymbolTable.cpp

213–214

This seems a bit hacky to me, as it always overwrites W.Wrap even though we wrote some data to W.Wrap before in another function. Can you remove that code to initialize W.Wrap?

[matthew.koontz]

ELF/SymbolTable.cpp

213–214

I'm confused as to what you mean. Are you talking about line 202? That is still needed to ensure the change I made to Relocations.cpp works properly.

I agree this is kinda hacky. The problem is the relocation code affects these symbols in a way that needs to be shared between them, but the relocation code also needs to know about the wrapping. We could avoid the memcpy here and just copy the fields that have been changed, which would be more clear but prone to breaking if fields are added. I'm open to other suggestions as well.

[grimar]

ELF/SymbolTable.cpp

213–214

How many fields need to be copied now if you avoid doing memcpy? I guess it might be a bit cleaner way.

[grimar]

test/ELF/wrap-plt.s

36

mov 1 and mov 2 does not look useful. Please use nop instead, that is more common and consistent with other tests.

[ruiu]

This problem is tricky. The issue is that when we rename wrap_foo -> foo by overwriting foo with wrap_foo's contents, we now have two copies of the same symbol, foo and wrap_foo. There might be some way to avoid this duplication, but I'm not completely sure how we can do that. Maybe this patch is the best way to fix it, but I want to think harder about it. Let me think more about it tomorrow.

[ruiu]

I thought about this patch for a bit today. Could you give a program that crashes without this patch? I don't think I understand if we really need this.

My understanding of this patch is this:

1. When you add --wrap=foo, foo is renamed real_foo, and wrap_foo renamed foo. After the renaming operation, wrap_foo and foo points to the same function. We do that by overwriting foo with wrap_foo's symbol contents.
2. In some cases, only one of foo or wrap_foo got a PLT entry.
3. If you call a function that didn't get a PLT entry, that call will be a direct call (i.e. without going through the PLT entry).

I don't know how (3) can lead to a crash. Could you explain it for me?

[matthew.koontz]

I've attached a basic program that will crash. It looks like this only happens when creating a shared object file, otherwise it gets a direct call like you describe. The problem is the applySymbolWrap duplicates __wrap_sym, and then sets the IsUsedInRegularObj in one of them to false. From then on, these get treated as two separate symbols. I think the cause of the runtime crash is setting the IsUsedInRegularObj prevents adding an entry in the dynamic symbol table, so the lookup fails. Also, the former code creates two PLT entries for __wrap_sym, when they should share a PLT entry since they really are the same symbol.

Ideally, we wouldn't do this memcpy and somehow have all of the references to sym to now point to __wrap_sym, but I can't think of a good way of doing that with the current code.

lld-test.tar.bz2390 BDownload

[ruiu]

Thank you for the test files. For convenience, I copy the contents here.

$ cat main.c
void test();
int main() { test(); return 0; }

$ cat test.c
int foo() { return 1; }
int wrap_foo() { return 0; }
void test() { foo(); wrap_foo(); }

$ cat run-test
#! /bin/bash
set -x
set -e
clang -fuse-ld=lld -Wl,--wrap=strstr -glldb -Wl,--wrap=foo -shared -o
test.so -fpic test.c
clang -fuse-ld=lld -Wl,--wrap=strstr -glldb -Wl,--wrap=foo -dynamic -o main
-fpic main.c ./test.so
./main

I'm not still very happy about this, but it seems like this is indeed the
only thing we can do with the current architecture. LGTM with some nits.
Thank you for finding and fixing this one!

[ruiu]

LGTM

ELF/SymbolTable.cpp

211

This function doesn't really apply relocations, so the name doesn't seem right. I'd name this applySymbolWrapReloc.

[matthew.koontz]

Addressed code review comments

• Renamed applyRelocationToWrappedSymbols -> applySymbolWrapReloc
• Updated test to use nop instead of mov

[ruiu]

Do you have commit access?

[matthew.koontz]

No, I do not have commit access

[ruiu]

I'll commit this patch for you.

[phosek]

This has introduced a regression when linking ASan instrumented code on AArch64: https://bugs.llvm.org/show_bug.cgi?id=38170. This is currently preventing us from rolling a new toolchain into Fuchsia. Can anyone take a look? I haven't yet figured out what's the cause for that issue.

ELF/Relocations.cpp

static void scanReloc(InputSectionBase &Sec, OffsetGetter &GetOffset, RelTy *&I,
   // Note that this function does not handle all TLS relocations.
   if (unsigned Processed =
           handleTlsRelocation<ELFT>(Type, Sym, Sec, Offset, Addend, Expr)) {
     I += (Processed - 1);
     return;
   }
 
   // If a relocation needs PLT, we create PLT and GOTPLT slots for the symbol.
-  if (needsPlt(Expr) && !Sym.isInPlt()) {
+  if (needsPlt(Expr) && !Sym.isInPlt() && Sym.IsUsedInRegularObj) {
     if (Sym.isGnuIFunc() && !Sym.IsPreemptible)
       addPltEntry<ELFT>(InX::Iplt, InX::IgotPlt, InX::RelaIplt,
                         Target->IRelativeRel, Sym);
     else
       addPltEntry<ELFT>(InX::Plt, InX::GotPlt, InX::RelaPlt, Target->PltRel,
                         Sym);
   }
 

ELF/SymbolTable.h

 // add*() functions, which are called by input files as they are parsed. There
 // is one add* function per symbol type.
 class SymbolTable {
 public:
   template <class ELFT> void addFile(InputFile *File);
   template <class ELFT> void addCombinedLTOObject();
   template <class ELFT> void addSymbolWrap(StringRef Name);
   void applySymbolWrap();
+  void applySymbolWrapReloc();
 
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
 
   Defined *addAbsolute(StringRef Name,
                        uint8_t Visibility = llvm::ELF::STV_HIDDEN,
                        uint8_t Binding = llvm::ELF::STB_GLOBAL);
 
   template <class ELFT> Symbol *addUndefined(StringRef Name);

ELF/SymbolTable.cpp

for (WrappedSymbol &W : WrappedSymbols) {
     // We now have two copies of __wrap_sym. Drop one.
     W.Wrap->IsUsedInRegularObj = false;
 
     if (Real)
       SymVector.push_back(Real);
   }
 }
 
+// Apply changes caused by relocations to wrapped symbols
+// This is needed for direct calls to __wrap_sym
+void SymbolTable::applySymbolWrapReloc() {
+  for (WrappedSymbol &W : WrappedSymbols) {
+    memcpy(W.Wrap, W.Sym, sizeof(SymbolUnion));
+
+    // Keep this so that this copy of the symbol remains dropped
+    W.Wrap->IsUsedInRegularObj = false;
+  }
+}
+
 static uint8_t getMinVisibility(uint8_t VA, uint8_t VB) {
   if (VA == STV_DEFAULT)
     return VB;
   if (VB == STV_DEFAULT)
     return VA;
   return std::min(VA, VB);
 }
 

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
   for (Symbol *S : Symtab->getSymbols())
     S->IsPreemptible |= computeIsPreemptible(*S);
 
   // Scan relocations. This must be done after every symbol is declared so that
   // we can correctly decide if a dynamic relocation is needed.
   if (!Config->Relocatable)
     forEachRelSec(scanRelocations<ELFT>);
 
+  // Apply changes caused by relocations to wrapped symbols
+  Symtab->applySymbolWrapReloc();
+
   if (InX::Plt && !InX::Plt->empty())
     InX::Plt->addSymbols();
   if (InX::Iplt && !InX::Iplt->empty())
     InX::Iplt->addSymbols();
 
   // Now that we have defined all possible global symbols including linker-
   // synthesized ones. Visit all symbols to give the finishing touches.
   for (Symbol *Sym : Symtab->getSymbols()) {

test/ELF/wrap-plt.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t
+
+// RUN: ld.lld -o %t2 %t -wrap foo -shared
+// RUN: llvm-readobj -s -r %t2 | FileCheck %s
+// RUN: llvm-objdump -d %t2 | FileCheck --check-prefix=DISASM %s
+
+// CHECK:      Name: .plt
+// CHECK-NEXT: Type: SHT_PROGBITS
+// CHECK-NEXT: Flags [
+// CHECK-NEXT:   SHF_ALLOC
+// CHECK-NEXT:   SHF_EXECINSTR
+// CHECK-NEXT: ]
+// CHECK-NEXT: Address: 0x1020
+// CHECK-NEXT: Offset:
+// CHECK-NEXT: Size: 48
+// CHECK-NEXT: Link: 0
+// CHECK-NEXT: Info: 0
+// CHECK-NEXT: AddressAlignment: 16
+
+// CHECK:      Relocations [
+// CHECK-NEXT:   Section ({{.*}}) .rela.plt {
+// CHECK-NEXT:     0x2018 R_X86_64_JUMP_SLOT __wrap_foo 0x0
+// CHECK-NEXT:     0x2020 R_X86_64_JUMP_SLOT _start 0x0
+// CHECK-NEXT:   }
+// CHECK-NEXT: ]
+
+// DISASM:      _start:
+// DISASM-NEXT: jmp 41
+// DISASM-NEXT: jmp 36
+// DISASM-NEXT: jmp 47
+
+.global foo
+foo:
+  nop
+
+.global __wrap_foo
+__wrap_foo:
+  nop
+
+.global _start
+_start:
+  jmp foo@plt
+  jmp __wrap_foo@plt
+  jmp _start@plt