Page MenuHomePhabricator

[LLD][ELF] Allow empty (.init|.preinit|.fini)_array to be RELRO
ClosedPublic

Authored by psmith on Mar 27 2020, 3:35 AM.

Details

Summary

The default GNU linker script uses the following idiom for the array sections. I'll use .init_array here, but this also applies to .preinit_array and .fini_array sections.

.init_array    :
{
  PROVIDE_HIDDEN (__init_array_start = .);
  KEEP (*(.init_array))
  PROVIDE_HIDDEN (__init_array_end = .);
}

The C-library will take references to the _start and _end symbols to process the array. This will make LLD keep the OutputSection even if there are no .init_array sections. As the current check for RELRO uses the section type for .init_array the above example with no .init_array InputSections fails the checks as there are no .init_array sections to give the OutputSection a type of SHT_INIT_ARRAY. This often leads to a non-contiguous RELRO error message, which I've seen in at least PR https://bugs.llvm.org/show_bug.cgi?id=44698 when suggesting a linker script as a work-around.

The simple fix is to a textual section match as well as a section type match. Potential alternatives involve setting the type of the name of an OutputSection called .init_array to SHT_INIT_ARRAY somewhere else. Or potentially trying to work out if an OutputSection has no content and can be considered RELRO regardless of its name.

Diff Detail

Event Timeline

psmith created this revision.Mar 27 2020, 3:35 AM
MaskRay added inline comments.Mar 28 2020, 9:44 AM
lld/test/ELF/relro-init-fini-script.s
20

It is not very clear what the 3 CHECK lines are about...

See pre_init_fini_array.s (D76852)

Suggest:

// RUN: llvm-readelf -S %t.so | FileCheck %s

// CHECK:      Name           Type     Address
// CHECK:      .preinit_array PROGBITS {{0+}}[[# %x,ADDR:]]
// CHECK-NEXT: .init_array    PROGBITS {{0+}}[[# ADDR]]
// CHECK-NEXT: .fini_array    PROGBITS {{0+}}[[# ADDR]]
// CHECK-NEXT: .data.rel.ro   PROGBITS {{0+}}[[# ADDR]]
24

Nit: the indentation is not needed.

psmith updated this revision to Diff 253627.Mar 30 2020, 10:12 AM

Thanks for the comments, I've added some tests to check that the _array sections are zero size, also to show the extent of the RELRO includes the section before and after.

MaskRay accepted this revision.Mar 30 2020, 10:28 AM

Looks great!

This revision is now accepted and ready to land.Mar 30 2020, 10:28 AM
grimar added inline comments.Mar 31 2020, 2:25 AM
lld/test/ELF/relro-init-fini-script.s
15

I`d suggest using llvm-readelf -S -segments then:
there is no need to have both llvm-readelf and llvm-readobj calls I think.

(Then perhaps you will be able to use {{0+}}[[# ADDR]] check to test PT_GNU_RELRO`s VA).

33

Missing an empty line after before ".section .data.rel.ro ...."

grimar accepted this revision.Mar 31 2020, 2:29 AM

LGTM with 2 nits.

lld/test/ELF/relro-init-fini-script.s
15

(Then perhaps you will be able to use {{0+}}# ADDR check to test PT_GNU_RELRO`s VA).

Ah, though it is not needed, with llvm-readelf -segments you can just test the same what is now tested with llvm-readobj: VA and size of the PT_GNU_RELRO.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 5:00 AM