[PATCH] D40735: [LLD][ELF] InX::BssRelRo should check section contents before marking relro

[Rafael Avila de Espindola via llvm-commits]

LGTM with the test cleanup that James suggested.

Thanks,
Rafael


Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith updated this revision to Diff 125331.
> peter.smith added a comment.
>
> Thanks for the comment, I've adapted the test to include parts of the previous error if mix .bss and .bss.rel.ro. I'm not sure if it adds much value in having two separate test cases, but do let me know if you'd like them split up?
>
>
> https://reviews.llvm.org/D40735
>
> Files:
>   ELF/Writer.cpp
>   test/ELF/relro-copyrel-bss-script.s
>
>
> Index: test/ELF/relro-copyrel-bss-script.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/relro-copyrel-bss-script.s
> @@ -0,0 +1,43 @@
> +// REQUIRES: x86
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t.o
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/copy-in-shared.s -o %t2.o
> +// RUN: ld.lld -shared %t.o %t2.o -o %t.so
> +
> +// A linker script that will map .bss.rel.ro into .bss.
> +// RUN: echo "SECTIONS { \
> +// RUN: .bss : { *(.bss) *(.bss.*) } \
> +// RUN: } " > %t.script
> +
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
> +// RUN: ld.lld %t3.o %t.so -z relro -o %t --script=%t.script 2>&1
> +// RUN: llvm-readobj --program-headers %t | FileCheck %s
> +        .section .text, "ax", @progbits
> +        .global bar
> +        .global foo
> +        .global _start
> +_start:
> +        callq bar
> +        // Will produce .bss.rel.ro that will match in .bss, this will lose
> +        // the relro property of the copy relocation.
> +        .quad foo
> +
> +        .data
> +        .space 4
> +
> +        // Non relro bss
> +        .bss
> +        // make large enough to affect PT_GNU_RELRO MemSize if this was marked
> +        // as relro.
> +        .space 0x2000
> +
> +// CHECK:     Type: PT_GNU_RELRO (0x6474E552)
> +// CHECK-NEXT:     Offset:
> +// CHECK-NEXT:     VirtualAddress:
> +// CHECK-NEXT:     PhysicalAddress:
> +// CHECK-NEXT:     FileSize:
> +// CHECK-NEXT:     MemSize: 4096
> +// CHECK-NEXT:     Flags [ (0x4)
> +// CHECK-NEXT:       PF_R (0x4)
> +// CHECK-NEXT:     ]
> +// CHECK-NEXT:     Alignment: 1
> +// CHECK-NEXT:   }
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -576,20 +576,14 @@
>    if (Sec == InX::Dynamic->getParent())
>      return true;
>  
> -  // .bss.rel.ro is used for copy relocations for read-only symbols.
> -  // Since the dynamic linker needs to process copy relocations, the
> -  // section cannot be read-only, but once initialized, they shouldn't
> -  // change.
> -  if (Sec == InX::BssRelRo->getParent())
> -    return true;
> -
>    // Sections with some special names are put into RELRO. This is a
>    // bit unfortunate because section names shouldn't be significant in
>    // ELF in spirit. But in reality many linker features depend on
>    // magic section names.
>    StringRef S = Sec->Name;
> -  return S == ".data.rel.ro" || S == ".ctors" || S == ".dtors" || S == ".jcr" ||
> -         S == ".eh_frame" || S == ".openbsd.randomdata";
> +  return S == ".data.rel.ro" || S == ".bss.rel.ro" || S == ".ctors" ||
> +         S == ".dtors" || S == ".jcr" || S == ".eh_frame" ||
> +         S == ".openbsd.randomdata";
>  }
>  
>  // We compute a rank for each section. The rank indicates where the
>
>
> Index: test/ELF/relro-copyrel-bss-script.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/relro-copyrel-bss-script.s
> @@ -0,0 +1,43 @@
> +// REQUIRES: x86
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t.o
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/copy-in-shared.s -o %t2.o
> +// RUN: ld.lld -shared %t.o %t2.o -o %t.so
> +
> +// A linker script that will map .bss.rel.ro into .bss.
> +// RUN: echo "SECTIONS { \
> +// RUN: .bss : { *(.bss) *(.bss.*) } \
> +// RUN: } " > %t.script
> +
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
> +// RUN: ld.lld %t3.o %t.so -z relro -o %t --script=%t.script 2>&1
> +// RUN: llvm-readobj --program-headers %t | FileCheck %s
> +        .section .text, "ax", @progbits
> +        .global bar
> +        .global foo
> +        .global _start
> +_start:
> +        callq bar
> +        // Will produce .bss.rel.ro that will match in .bss, this will lose
> +        // the relro property of the copy relocation.
> +        .quad foo
> +
> +        .data
> +        .space 4
> +
> +        // Non relro bss
> +        .bss
> +        // make large enough to affect PT_GNU_RELRO MemSize if this was marked
> +        // as relro.
> +        .space 0x2000
> +
> +// CHECK:     Type: PT_GNU_RELRO (0x6474E552)
> +// CHECK-NEXT:     Offset:
> +// CHECK-NEXT:     VirtualAddress:
> +// CHECK-NEXT:     PhysicalAddress:
> +// CHECK-NEXT:     FileSize:
> +// CHECK-NEXT:     MemSize: 4096
> +// CHECK-NEXT:     Flags [ (0x4)
> +// CHECK-NEXT:       PF_R (0x4)
> +// CHECK-NEXT:     ]
> +// CHECK-NEXT:     Alignment: 1
> +// CHECK-NEXT:   }
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -576,20 +576,14 @@
>    if (Sec == InX::Dynamic->getParent())
>      return true;
>  
> -  // .bss.rel.ro is used for copy relocations for read-only symbols.
> -  // Since the dynamic linker needs to process copy relocations, the
> -  // section cannot be read-only, but once initialized, they shouldn't
> -  // change.
> -  if (Sec == InX::BssRelRo->getParent())
> -    return true;
> -
>    // Sections with some special names are put into RELRO. This is a
>    // bit unfortunate because section names shouldn't be significant in
>    // ELF in spirit. But in reality many linker features depend on
>    // magic section names.
>    StringRef S = Sec->Name;
> -  return S == ".data.rel.ro" || S == ".ctors" || S == ".dtors" || S == ".jcr" ||
> -         S == ".eh_frame" || S == ".openbsd.randomdata";
> +  return S == ".data.rel.ro" || S == ".bss.rel.ro" || S == ".ctors" ||
> +         S == ".dtors" || S == ".jcr" || S == ".eh_frame" ||
> +         S == ".openbsd.randomdata";
>  }
>  
>  // We compute a rank for each section. The rank indicates where the