Skip to content

Commit

Permalink
[asan] Ensure __asan_register_elf_globals is called in COMDAT asan.mo…
Browse files Browse the repository at this point in the history 
…dule_ctor (#67745)

On ELF platforms, when there is no global variable and the unique module ID is
non-empty, COMDAT asan.module_ctor is created with no
`__asan_register_elf_globals` calls. If this COMDAT is the prevailing copy
selected by the linker, the linkage unit will have no
`__asan_register_elf_globals` call: the redzone will not be poisoned and ODR
violation checker will not work (#67677).

This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call `__asan_register_globals`
(`InstrumentGlobalsWithMetadataArray`) do not use COMDAT.

To fix #67677:

* Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
* Call `__asan_register_elf_globals` even if there is no global variable.
* If the unique module ID is empty, don't call SetComdatForGlobalMetadata:
  placing `@.str` in a COMDAT would incorrectly discard internal COMDAT `@.str`
  in other compile units.

Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call `__asan_register_elf_globals`. However, the asan.module_ctor
function cannot be eliminated by the linker.

Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.
```
echo > a.cc  # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc  # with global variable, with uniqueModuleId
echo 'int g;' > c.cc  # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
  for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
    for f in a.cc b.cc c.cc; do
      echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
      /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
    done
  done
done
```
  • Loading branch information
@MaskRay
MaskRay committed Sep 29, 2023
1 parent d89d3a6 commit 16eed8c
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 37 deletions.
61 changes: 32 additions & 29 deletions llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -819,11 +819,11 @@ class ModuleAddressSanitizer {
private:
void initializeCallbacks(Module &M);

bool InstrumentGlobals(IRBuilder<> &IRB, Module &M, bool *CtorComdat);
void instrumentGlobals(IRBuilder<> &IRB, Module &M, bool *CtorComdat);
void InstrumentGlobalsCOFF(IRBuilder<> &IRB, Module &M,
ArrayRef<GlobalVariable *> ExtendedGlobals,
ArrayRef<Constant *> MetadataInitializers);
void InstrumentGlobalsELF(IRBuilder<> &IRB, Module &M,
void instrumentGlobalsELF(IRBuilder<> &IRB, Module &M,
ArrayRef<GlobalVariable *> ExtendedGlobals,
ArrayRef<Constant *> MetadataInitializers,
const std::string &UniqueModuleId);
Expand Down Expand Up @@ -2177,7 +2177,7 @@ void ModuleAddressSanitizer::InstrumentGlobalsCOFF(
appendToCompilerUsed(M, MetadataGlobals);
}

void ModuleAddressSanitizer::InstrumentGlobalsELF(
void ModuleAddressSanitizer::instrumentGlobalsELF(
IRBuilder<> &IRB, Module &M, ArrayRef<GlobalVariable *> ExtendedGlobals,
ArrayRef<Constant *> MetadataInitializers,
const std::string &UniqueModuleId) {
Expand All @@ -2187,7 +2187,7 @@ void ModuleAddressSanitizer::InstrumentGlobalsELF(
// false negative odr violations at link time. If odr indicators are used, we
// keep the comdat sections, as link time odr violations will be dectected on
// the odr indicator symbols.
bool UseComdatForGlobalsGC = UseOdrIndicator;
bool UseComdatForGlobalsGC = UseOdrIndicator && !UniqueModuleId.empty();

SmallVector<GlobalValue *, 16> MetadataGlobals(ExtendedGlobals.size());
for (size_t i = 0; i < ExtendedGlobals.size(); i++) {
Expand Down Expand Up @@ -2237,7 +2237,7 @@ void ModuleAddressSanitizer::InstrumentGlobalsELF(

// We also need to unregister globals at the end, e.g., when a shared library
// gets closed.
if (DestructorKind != AsanDtorKind::None) {
if (DestructorKind != AsanDtorKind::None && !MetadataGlobals.empty()) {
IRBuilder<> IrbDtor(CreateAsanModuleDtor(M));
IrbDtor.CreateCall(AsanUnregisterElfGlobals,
{IRB.CreatePointerCast(RegisteredFlag, IntptrTy),
Expand Down Expand Up @@ -2343,10 +2343,8 @@ void ModuleAddressSanitizer::InstrumentGlobalsWithMetadataArray(
// redzones and inserts this function into llvm.global_ctors.
// Sets *CtorComdat to true if the global registration code emitted into the
// asan constructor is comdat-compatible.
bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
void ModuleAddressSanitizer::instrumentGlobals(IRBuilder<> &IRB, Module &M,
bool *CtorComdat) {
*CtorComdat = false;

// Build set of globals that are aliased by some GA, where
// getExcludedAliasedGlobal(GA) returns the relevant GlobalVariable.
SmallPtrSet<const GlobalVariable *, 16> AliasedGlobalExclusions;
Expand All @@ -2364,11 +2362,6 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
}

size_t n = GlobalsToChange.size();
if (n == 0) {
*CtorComdat = true;
return false;
}

auto &DL = M.getDataLayout();

// A global is described by a structure
Expand All @@ -2391,8 +2384,11 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,

// We shouldn't merge same module names, as this string serves as unique
// module ID in runtime.
GlobalVariable *ModuleName = createPrivateGlobalForString(
M, M.getModuleIdentifier(), /*AllowMerging*/ false, kAsanGenPrefix);
GlobalVariable *ModuleName =
n != 0
? createPrivateGlobalForString(M, M.getModuleIdentifier(),
/*AllowMerging*/ false, kAsanGenPrefix)
: nullptr;

for (size_t i = 0; i < n; i++) {
GlobalVariable *G = GlobalsToChange[i];
Expand Down Expand Up @@ -2517,27 +2513,34 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
}
appendToCompilerUsed(M, ArrayRef<GlobalValue *>(GlobalsToAddToUsedList));

std::string ELFUniqueModuleId =
(UseGlobalsGC && TargetTriple.isOSBinFormatELF()) ? getUniqueModuleId(&M)
: "";

if (!ELFUniqueModuleId.empty()) {
InstrumentGlobalsELF(IRB, M, NewGlobals, Initializers, ELFUniqueModuleId);
if (UseGlobalsGC && TargetTriple.isOSBinFormatELF()) {
// Use COMDAT and register globals even if n == 0 to ensure that (a) the
// linkage unit will only have one module constructor, and (b) the register
// function will be called. The module destructor is not created when n ==
// 0.
*CtorComdat = true;
} else if (UseGlobalsGC && TargetTriple.isOSBinFormatCOFF()) {
InstrumentGlobalsCOFF(IRB, M, NewGlobals, Initializers);
} else if (UseGlobalsGC && ShouldUseMachOGlobalsSection()) {
InstrumentGlobalsMachO(IRB, M, NewGlobals, Initializers);
instrumentGlobalsELF(IRB, M, NewGlobals, Initializers,
getUniqueModuleId(&M));
} else if (n == 0) {
// When UseGlobalsGC is false, COMDAT can still be used if n == 0, because
// all compile units will have identical module constructor/destructor.
*CtorComdat = TargetTriple.isOSBinFormatELF();
} else {
InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
*CtorComdat = false;
if (UseGlobalsGC && TargetTriple.isOSBinFormatCOFF()) {
InstrumentGlobalsCOFF(IRB, M, NewGlobals, Initializers);
} else if (UseGlobalsGC && ShouldUseMachOGlobalsSection()) {
InstrumentGlobalsMachO(IRB, M, NewGlobals, Initializers);
} else {
InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
}
}

// Create calls for poisoning before initializers run and unpoisoning after.
if (HasDynamicallyInitializedGlobals)
createInitializerPoisonCalls(M, ModuleName);

LLVM_DEBUG(dbgs() << M);
return true;
}

uint64_t
Expand Down Expand Up @@ -2601,10 +2604,10 @@ bool ModuleAddressSanitizer::instrumentModule(Module &M) {
assert(AsanCtorFunction || ConstructorKind == AsanCtorKind::None);
if (AsanCtorFunction) {
IRBuilder<> IRB(AsanCtorFunction->getEntryBlock().getTerminator());
InstrumentGlobals(IRB, M, &CtorComdat);
instrumentGlobals(IRB, M, &CtorComdat);
} else {
IRBuilder<> IRB(*C);
InstrumentGlobals(IRB, M, &CtorComdat);
instrumentGlobals(IRB, M, &CtorComdat);
}
}

Expand Down
4 changes: 3 additions & 1 deletion llvm/test/Instrumentation/AddressSanitizer/basic.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,10 @@ define void @test_swifterror_3() sanitize_address {

;; ctor/dtor have the nounwind attribute. See uwtable.ll, they additionally have
;; the uwtable attribute with the module flag "uwtable".
; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] {{(comdat )?}}{
; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] comdat {
; CHECK: call void @__asan_init()
;; __asan_register_elf_globals is called even if this module does not contain instrumented global variables.
; CHECK: call void @__asan_register_elf_globals(i64 ptrtoint (ptr @___asan_globals_registered to i64), i64 ptrtoint (ptr @__start_asan_globals to i64), i64 ptrtoint (ptr @__stop_asan_globals to i64))

; CHECK: attributes #[[#ATTR]] = { nounwind }

Expand Down
19 changes: 15 additions & 4 deletions llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-apple-macosx10.11.0 -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-pc-windows-msvc19.0.24215 -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -asan-mapping-scale=5 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefixes=CHECK,CHECK-S5 %s
; RUN: rm -rf %t && split-file %s %t && cd %t
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-apple-macosx10.11.0 -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-pc-windows-msvc19.0.24215 -S | FileCheck --check-prefix=CHECK %s
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -asan-mapping-scale=5 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefixes=CHECK,CHECK-S5 %s

; RUN: opt < empty.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=ELF-NOGC %s

;--- a.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

; Globals:
Expand Down Expand Up @@ -59,3 +63,10 @@ attributes #1 = { nounwind sanitize_address "less-precise-fpmad"="false" "frame-
!7 = !{!"/tmp/asan-globals.cpp", i32 7, i32 5}
!8 = !{!"/tmp/asan-globals.cpp", i32 12, i32 14}
!9 = !{!"/tmp/asan-globals.cpp", i32 14, i32 25}

;; In the presence of instrumented global variables, asan.module_ctor do not use comdat.
; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] {

; ELF-NOGC: define internal void @asan.module_ctor() #[[#ATTR:]] comdat {

;--- empty.ll
50 changes: 47 additions & 3 deletions llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,14 @@
; We keep using comdats for garbage collection if odr indicators are
; enabled as indicator symbols will cause link time odr violations.
; This is to fix PR 47925.
;
; RUN: opt < %s -passes=asan -asan-globals-live-support=1 -asan-use-odr-indicator=0 -S | FileCheck %s --check-prefixes=CHECK,NOCOMDAT
; RUN: rm -rf %t && split-file %s %t && cd %t
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=1 -asan-use-odr-indicator=0 -S | FileCheck %s --check-prefixes=CHECK,NOCOMDAT
; Check that enabling odr indicators enables comdat for globals.
; RUN: opt < %s -passes=asan -asan-globals-live-support=1 -S | FileCheck %s --check-prefixes=CHECK,COMDAT
; RUN: opt < a.ll -passes=asan -asan-globals-live-support=1 -S | FileCheck %s --check-prefixes=CHECK,COMDAT

; RUN: opt < no_module_id.ll -passes=asan -S | FileCheck %s --check-prefix=NOMODULEID

;--- a.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

Expand Down Expand Up @@ -87,3 +91,43 @@ attributes #1 = { nounwind sanitize_address "less-precise-fpmad"="false" "frame-
!7 = !{!"/tmp/asan-globals.cpp", i32 7, i32 5}
!8 = !{!"/tmp/asan-globals.cpp", i32 12, i32 14}
!9 = !{!"/tmp/asan-globals.cpp", i32 14, i32 25}

;--- no_module_id.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; NOMODULEID: $asan.module_ctor = comdat any
; NOMODULEID: $asan.module_dtor = comdat any

;; Don't place the instrumented globals in a comdat when the unique module ID is empty.
; NOMODULEID: @.str = internal constant { [4 x i8], [28 x i8] } { [4 x i8] c"str\00", [28 x i8] zeroinitializer }, align 32
; NOMODULEID: @_ZL3buf = internal global { [4 x i8], [28 x i8] } zeroinitializer, align 32
; NOMODULEID: @__asan_global_.str = private global {{.*}}, section "asan_globals", !associated !0
; NOMODULEID: @__asan_global__ZL3buf = private global {{.*}}, section "asan_globals", !associated !1
; NOMODULEID: @llvm.compiler.used = appending global [4 x ptr] [ptr @.str, ptr @_ZL3buf, ptr @__asan_global_.str, ptr @__asan_global__ZL3buf]

; NOMODULEID: define internal void @asan.module_ctor() #[[#]] comdat {
; NOMODULEID-NEXT: call void @__asan_init()
; NOMODULEID-NEXT: call void @__asan_version_mismatch_check_v8()
; NOMODULEID-NEXT: call void @__asan_register_elf_globals(i64 ptrtoint (ptr @___asan_globals_registered to i64), i64 ptrtoint (ptr @__start_asan_globals to i64), i64 ptrtoint (ptr @__stop_asan_globals to i64))
; NOMODULEID-NEXT: ret void
; NOMODULEID-NEXT: }

; NOMODULEID: !0 = !{ptr @.str}
; NOMODULEID: !1 = !{ptr @_ZL3buf}

@.str = private unnamed_addr constant [4 x i8] c"str\00", align 1
@_ZL3buf = internal unnamed_addr global [4 x i8] zeroinitializer, align 1
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I_a.cc, ptr null }]

declare void @ext(ptr noundef)

; Function Attrs: uwtable
define internal void @_GLOBAL__sub_I_a.cc() #2 section ".text.startup" {
entry:
%0 = load i8, ptr @_ZL3buf, align 1
%inc = add i8 %0, 1
store i8 %inc, ptr @_ZL3buf, align 1
tail call void @ext(ptr noundef nonnull @.str)
ret void
}

0 comments on commit 16eed8c

Please sign in to comment.