[ELF] Do not produce DT_JMPREL and DT_PLTGOT if .rela.plt is empty.

[ikudrin]

Summary

If .rela.plt is mentioned in a linker script, it might be preserved even if it is empty. In that case, LLD created DT_JMPREL and DT_PLTGOT dynamic tags. When the tags exist, a dynamic loader writes values into reserved slots in .got.plt to support lazy symbol resolution. The problem is that, in fact, the linker has not reserved that space, and the writing may occur into the memory allocated for something else.

[ikudrin]

On Linux, the issue can be reproduced in the following way:

// d.c
long val[3];
long getval(int i) {return val[i];}

// main.c
#include <stdio.h>

long getval(int i);

int main() {
  for (int i = 0; i < 3; ++i)
    printf("val[%d] = %ld\n", i, getval(i));
  return 0;
}

# d.t
PHDRS {
    all PT_LOAD;
    dyn PT_DYNAMIC;
}
SECTIONS {
    .rela.plt : {*(.rela.plt)}: all
    .dynamic : {*(.dynamic)}: all : dyn
    .got.plt : {*(.got.plt)}: all
    .bss : {*(.bss)}: all
}

$ clang d.c -nostdlib -fPIC -shared -o d.so -fuse-ld=lld -Wl,-T,d.t
$ clang main.c ./d.so -fpie -o test -fuse-ld=lld
$ ./test
val[0] = 0
val[1] = 140495953956864
val[2] = 140495951804032

[ruiu]

LGTM

[grimar]

I think this test case should live in test\ELF\linkerscript folder. Also it should be .test, not .s,
like our other tests that has no or little amount of asm code.
(See unused-synthetic2.test for example)

[MaskRay]

test/ELF/empty-relaplt-dyntags.s

1 ↗

(On Diff #206817)

This test should probably be moved to linkerscript/

5 ↗

(On Diff #206817)

Nit: indent the string literal so that S in SECTIONS does not occupy the same column of e in echo.

[MaskRay]

test/ELF/empty-relaplt-dyntags.s

18 ↗

(On Diff #206817)

to write into memory it considers reserved

I don't think a dynamic linker should do this because DT_PLTRELSZ is zero.

I agree that 0 JMPREL should be omitted.

0x0000000000000017 (JMPREL) 0x0
0x0000000000000002 (PLTRELSZ) 0 (bytes)
0x0000000000000003 (PLTGOT) 0x0

[ikudrin]

• Reworked the test.

I am not sure about moving it into linkerscript, though, because it does not test the support for linker scripts per se. Moreover, there are lots of tests in the main test folder which also use linker scripts.

[ikudrin]

test/ELF/empty-relaplt-dyntags.s

18 ↗

(On Diff #206817)

That is why I posted the reproducer. I tested it on Ubuntu 18.04 with the fresh built lld.

[grimar]

In D63869#1560610, @ikudrin wrote:

• Reworked the test.

I am not sure about moving it into linkerscript, though, because it does not test the support for linker scripts per se.

Actually .rela.plt is kept only because it is mentioned in a linker script. It is relative.

Moreover, there are lots of tests in the main test folder which also use linker scripts.

And most of them are initially placed there by mistake it seems. Also, I found no other .test files that are based on linker script in that folder.
(.test files I see are YAML based and all linker script based test still lives in ELF/linkerscript).

[MaskRay]

test/ELF/empty-relaplt-dyntags.s

18 ↗

(On Diff #206817)

You are right :) glibc writes the slots unconditionally

.got.plt[1] = _dl_runtime_resolve, .got.plt[2] = link_map

I have no more comments if you can fix the echo indentation and move the test under linkerscript

Empty .rela.plt is kept due to the isDiscardable() logic in LinkerScript.cpp

[MaskRay]

test/ELF/empty-relaplt-dyntags.test

10 ↗

(On Diff #206829)

I think you need a readelf -S test to check that .rela.plt exists.

Then, for this comment, you may just say that we should not write a DT_JMPREL of 0, because otherwise the first few slots of the entries pointed to by DT_PLTGOT may be unexpected accessed at runtime.

The test doesn't add .got.plt (actually in glibc DT_PLTGOT will not be used if DT_JMPREL does not exist).

[ikudrin]

• Extended the test and moved it into the linkerscript folder.

[grimar]

LGTM

ELF/SyntheticSections.cpp

addInt(Config->UseAndroidRelrTags ? DT_ANDROID_RELRENT : DT_RELRENT,
            sizeof(Elf_Relr));
   }
   // .rel[a].plt section usually consists of two parts, containing plt and
   // iplt relocations. It is possible to have only iplt relocations in the
   // output. In that case RelaPlt is empty and have zero offset, the same offset
   // as RelaIplt have. And we still want to emit proper dynamic tags for that
   // case, so here we always use RelaPlt as marker for the begining of
   // .rel[a].plt section.
-  if (IsMain && In.RelaPlt->getParent()->isLive()) {
+  if (IsMain && (In.RelaPlt->isNeeded() || In.RelaIplt->isNeeded())) {
     addInSec(DT_JMPREL, In.RelaPlt);
     Entries.push_back({DT_PLTRELSZ, addPltRelSz});
     switch (Config->EMachine) {
     case EM_MIPS:
       addInSec(DT_MIPS_PLTGOT, In.GotPlt);
       break;
     case EM_SPARCV9:
       addInSec(DT_PLTGOT, In.Plt);

test/ELF/linkerscript/empty-relaplt-dyntags.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux /dev/null -o %t.o
+# RUN: ld.lld -shared %t.o -T %s -o %t
+# RUN: llvm-readobj --dynamic-table --sections %t | FileCheck %s
+
+## In spite of .rela.plt is empty, it might have been preserved because it is
+## mentioned in the linker script. However, even in that case, we should not
+## produce DT_JMPREL and DT_PLTGOT tags because this can cause a dynamic loader
+## to write into slots in .got.plt it considers reserved to support lazy symbol
+## resolution. In fact, as .got.plt is also empty, that memory might be
+## allocated for something else.
+
+# CHECK: Sections [
+# CHECK:      Name: .rela.plt
+# CHECK-NEXT: Type: SHT_RELA
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 0
+# CHECK:      Name: .got.plt
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_WRITE
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 0
+
+# CHECK: DynamicSection [
+# CHECK-NOT: JMPREL
+# CHECK-NOT: PLTGOT
+
+PHDRS {
+  all PT_LOAD;
+  dyn PT_DYNAMIC;
+}
+SECTIONS {
+  .rela.plt : { *(.rela.plt) }: all
+  .dynamic : { *(.dynamic) }: all : dyn
+  .got.plt : {*(.got.plt)}: all
+}