From 888437e1b60011b8a375dd30928ec925b448da57 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Wed, 11 Oct 2023 09:23:56 -0700 Subject: [asan] Ensure __asan_register_elf_globals is called in COMDAT asan.module_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 ``` --- Identical to commit 16eed8c906875e748c3cb610f3dc4b875f3882aa. 6420d3301cd4f0793adcf11f59e8398db73737d8 is an incorrect revert for genuine purely internal issues. --- .../Instrumentation/AddressSanitizer.cpp | 61 ++++++++++++---------- .../test/Instrumentation/AddressSanitizer/basic.ll | 4 +- .../AddressSanitizer/global_metadata_array.ll | 19 +++++-- .../AddressSanitizer/global_with_comdat.ll | 50 ++++++++++++++++-- 4 files changed, 97 insertions(+), 37 deletions(-) diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp index bde5fba20f3b..f4bf6db569f2 100644 --- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp +++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp @@ -819,11 +819,11 @@ public: 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 ExtendedGlobals, ArrayRef MetadataInitializers); - void InstrumentGlobalsELF(IRBuilder<> &IRB, Module &M, + void instrumentGlobalsELF(IRBuilder<> &IRB, Module &M, ArrayRef ExtendedGlobals, ArrayRef MetadataInitializers, const std::string &UniqueModuleId); @@ -2177,7 +2177,7 @@ void ModuleAddressSanitizer::InstrumentGlobalsCOFF( appendToCompilerUsed(M, MetadataGlobals); } -void ModuleAddressSanitizer::InstrumentGlobalsELF( +void ModuleAddressSanitizer::instrumentGlobalsELF( IRBuilder<> &IRB, Module &M, ArrayRef ExtendedGlobals, ArrayRef MetadataInitializers, const std::string &UniqueModuleId) { @@ -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 MetadataGlobals(ExtendedGlobals.size()); for (size_t i = 0; i < ExtendedGlobals.size(); i++) { @@ -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), @@ -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 AliasedGlobalExclusions; @@ -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 @@ -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]; @@ -2517,19 +2513,27 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M, } appendToCompilerUsed(M, ArrayRef(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. @@ -2537,7 +2541,6 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M, createInitializerPoisonCalls(M, ModuleName); LLVM_DEBUG(dbgs() << M); - return true; } uint64_t @@ -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); } } diff --git a/llvm/test/Instrumentation/AddressSanitizer/basic.ll b/llvm/test/Instrumentation/AddressSanitizer/basic.ll index 068d6d18cd45..2064db5a0918 100644 --- a/llvm/test/Instrumentation/AddressSanitizer/basic.ll +++ b/llvm/test/Instrumentation/AddressSanitizer/basic.ll @@ -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 } diff --git a/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll b/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll index 0e4d1f168d01..1617bf9b67aa 100644 --- a/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll +++ b/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll @@ -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: @@ -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 diff --git a/llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll b/llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll index 47bb1f102e2f..699b8287d358 100644 --- a/llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll +++ b/llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll @@ -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" @@ -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 +} -- cgit v1.2.3