From f26dfea1bf5da060aa895fa107575a228d5b0702 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Sat, 3 Aug 2024 10:35:20 -0700 Subject: [PATCH 1/8] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20to=20main=20this=20commit=20is=20based=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- compiler-rt/lib/asan/asan_globals.cpp | 108 +++++++++++++++++++------- 1 file changed, 80 insertions(+), 28 deletions(-) diff --git a/compiler-rt/lib/asan/asan_globals.cpp b/compiler-rt/lib/asan/asan_globals.cpp index cc5308a24fe89..293e771e1dc06 100644 --- a/compiler-rt/lib/asan/asan_globals.cpp +++ b/compiler-rt/lib/asan/asan_globals.cpp @@ -47,8 +47,6 @@ struct DynInitGlobal { bool initialized = false; DynInitGlobal *next = nullptr; }; -typedef IntrusiveList DynInitGlobals; -static DynInitGlobals dynamic_init_globals SANITIZER_GUARDED_BY(mu_for_globals); // We want to remember where a certain range of globals was registered. struct GlobalRegistrationSite { @@ -72,6 +70,25 @@ static ListOfGlobals &GlobalsByIndicator(uptr odr_indicator) return (*globals_by_indicator)[odr_indicator]; } +static const char *current_dynamic_init_module_name + SANITIZER_GUARDED_BY(mu_for_globals) = nullptr; + +using DynInitGlobalsByModule = + DenseMap>; + +// TODO: Add a NoDestroy helper, this patter is very common in sanitizers. +static DynInitGlobalsByModule &DynInitGlobals() + SANITIZER_REQUIRES(mu_for_globals) { + static DynInitGlobalsByModule *globals_by_module = nullptr; + if (!globals_by_module) { + alignas(alignof(DynInitGlobalsByModule)) static char + placeholder[sizeof(DynInitGlobalsByModule)]; + globals_by_module = new (placeholder) DynInitGlobalsByModule(); + } + + return *globals_by_module; +} + ALWAYS_INLINE void PoisonShadowForGlobal(const Global *g, u8 value) { FastPoisonShadow(g->beg, g->size_with_redzone, value); } @@ -94,6 +111,31 @@ static void AddGlobalToList(ListOfGlobals &list, const Global *g) { list.push_front(new (GetGlobalLowLevelAllocator()) GlobalListNode{g}); } +static void UnpoisonDynamicGlobals(IntrusiveList &dyn_globals, + bool mark_initialized) { + for (auto &dyn_g : dyn_globals) { + const Global *g = &dyn_g.g; + if (dyn_g.initialized) + continue; + // Unpoison the whole global. + PoisonShadowForGlobal(g, 0); + // Poison redzones back. + PoisonRedZones(*g); + if (mark_initialized) + dyn_g.initialized = true; + } +} + +static void PoisonDynamicGlobals( + const IntrusiveList &dyn_globals) { + for (auto &dyn_g : dyn_globals) { + const Global *g = &dyn_g.g; + if (dyn_g.initialized) + continue; + PoisonShadowForGlobal(g, kAsanInitializationOrderMagic); + } +} + static bool IsAddressNearGlobal(uptr addr, const __asan_global &g) { if (addr <= g.beg - kMinimalDistanceFromAnotherGlobal) return false; if (addr >= g.beg + g.size_with_redzone) return false; @@ -257,8 +299,8 @@ static void RegisterGlobal(const Global *g) SANITIZER_REQUIRES(mu_for_globals) { AddGlobalToList(list_of_all_globals, g); if (g->has_dynamic_init) { - dynamic_init_globals.push_back(new (GetGlobalLowLevelAllocator()) - DynInitGlobal{*g, false}); + DynInitGlobals()[g->module_name].push_back( + new (GetGlobalLowLevelAllocator()) DynInitGlobal{*g, false}); } } @@ -289,13 +331,10 @@ void StopInitOrderChecking() { return; Lock lock(&mu_for_globals); flags()->check_initialization_order = false; - for (const DynInitGlobal &dyn_g : dynamic_init_globals) { - const Global *g = &dyn_g.g; - // Unpoison the whole global. - PoisonShadowForGlobal(g, 0); - // Poison redzones back. - PoisonRedZones(*g); - } + DynInitGlobals().forEach([&](auto &kv) { + UnpoisonDynamicGlobals(kv.second, /*mark_initialized=*/false); + return true; + }); } static bool IsASCII(unsigned char c) { return /*0x00 <= c &&*/ c <= 0x7F; } @@ -456,17 +495,29 @@ void __asan_before_dynamic_init(const char *module_name) { CHECK(module_name); CHECK(AsanInited()); Lock lock(&mu_for_globals); + if (current_dynamic_init_module_name == module_name) + return; if (flags()->report_globals >= 3) Printf("DynInitPoison module: %s\n", module_name); - for (DynInitGlobal &dyn_g : dynamic_init_globals) { - const Global *g = &dyn_g.g; - if (dyn_g.initialized) - continue; - if (g->module_name != module_name) - PoisonShadowForGlobal(g, kAsanInitializationOrderMagic); - else if (!strict_init_order) - dyn_g.initialized = true; + + if (current_dynamic_init_module_name == nullptr) { + // First call, poison all globals from other modules. + DynInitGlobals().forEach([&](auto &kv) { + if (kv.first != module_name) { + PoisonDynamicGlobals(kv.second); + } else { + UnpoisonDynamicGlobals(kv.second, + /*mark_initialized=*/!strict_init_order); + } + return true; + }); + } else { + // Module changed. + PoisonDynamicGlobals(DynInitGlobals()[current_dynamic_init_module_name]); + UnpoisonDynamicGlobals(DynInitGlobals()[module_name], + /*mark_initialized=*/!strict_init_order); } + current_dynamic_init_module_name = module_name; } // This method runs immediately after dynamic initialization in each TU, when @@ -477,15 +528,16 @@ void __asan_after_dynamic_init() { return; CHECK(AsanInited()); Lock lock(&mu_for_globals); + if (!current_dynamic_init_module_name) + return; + if (flags()->report_globals >= 3) Printf("DynInitUnpoison\n"); - for (const DynInitGlobal &dyn_g : dynamic_init_globals) { - const Global *g = &dyn_g.g; - if (!dyn_g.initialized) { - // Unpoison the whole global. - PoisonShadowForGlobal(g, 0); - // Poison redzones back. - PoisonRedZones(*g); - } - } + + DynInitGlobals().forEach([&](auto &kv) { + UnpoisonDynamicGlobals(kv.second, /*mark_initialized=*/false); + return true; + }); + + current_dynamic_init_module_name = nullptr; } From 75060ddc588f64c6f262f73ae84f0d993111ae34 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Sat, 3 Aug 2024 13:01:36 -0700 Subject: [PATCH 2/8] reword comment Created using spr 1.3.4 --- compiler-rt/test/asan/TestCases/initialization-nobug.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler-rt/test/asan/TestCases/initialization-nobug.cpp b/compiler-rt/test/asan/TestCases/initialization-nobug.cpp index d5969b547ecb0..62d063b3a959e 100644 --- a/compiler-rt/test/asan/TestCases/initialization-nobug.cpp +++ b/compiler-rt/test/asan/TestCases/initialization-nobug.cpp @@ -46,7 +46,8 @@ int main() { return 0; } // CHECK: DynInitPoison // CHECK: DynInitPoison -// In general case we only need the last of DynInit{Poison,Unpoison} is always -// Unpoison. +// In general case entire set of DynInitPoison must be followed by at lest one +// DynInitUnpoison. In some cases we can limit number of DynInitUnpoison, see +// initialization-nobug-lld.cpp. // CHECK: DynInitUnpoison From 1ffa55d00c5756bb19404c239096c10333b43bec Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Sat, 3 Aug 2024 13:02:15 -0700 Subject: [PATCH 3/8] reword Created using spr 1.3.4 --- compiler-rt/test/asan/TestCases/initialization-nobug.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler-rt/test/asan/TestCases/initialization-nobug.cpp b/compiler-rt/test/asan/TestCases/initialization-nobug.cpp index 62d063b3a959e..f66d501124bc4 100644 --- a/compiler-rt/test/asan/TestCases/initialization-nobug.cpp +++ b/compiler-rt/test/asan/TestCases/initialization-nobug.cpp @@ -47,7 +47,7 @@ int main() { return 0; } // CHECK: DynInitPoison // In general case entire set of DynInitPoison must be followed by at lest one -// DynInitUnpoison. In some cases we can limit number of DynInitUnpoison, see -// initialization-nobug-lld.cpp. +// DynInitUnpoison. In some cases we can limit the number of DynInitUnpoison, +// see initialization-nobug-lld.cpp. // CHECK: DynInitUnpoison From c2ca3ba8984296f17aa61e65d5a0587db78bd832 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 7 Aug 2024 16:46:44 -0700 Subject: [PATCH 4/8] move Created using spr 1.3.4 --- .../asan/TestCases/{ => Linux}/initialization-nobug-lld.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename compiler-rt/test/asan/TestCases/{ => Linux}/initialization-nobug-lld.cpp (91%) diff --git a/compiler-rt/test/asan/TestCases/initialization-nobug-lld.cpp b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp similarity index 91% rename from compiler-rt/test/asan/TestCases/initialization-nobug-lld.cpp rename to compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp index ece3118de4ad9..80b502cd5538a 100644 --- a/compiler-rt/test/asan/TestCases/initialization-nobug-lld.cpp +++ b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp @@ -1,4 +1,4 @@ -// RUN: %clangxx_asan -O3 %p/initialization-nobug.cpp %p/Helpers/initialization-nobug-extra.cpp -fuse-ld=lld -o %t && %env_asan_opts=check_initialization_order=true:report_globals=3 %run %t 2>&1 | FileCheck %s --implicit-check-not "DynInit" +// RUN: %clangxx_asan -O3 %p/initialization-nobug.cpp %S/Helpers/initialization-nobug-extra.cpp -fuse-ld=lld -o %t && %env_asan_opts=check_initialization_order=true:report_globals=3 %run %t 2>&1 | FileCheck %s --implicit-check-not "DynInit" // Same as initialization-nobug.cpp, but with lld we expect just one // `DynInitUnpoison` executed after `AfterDynamicInit` at the end. From 317ad9c5bf55942aece582a884b629737cda7f08 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 7 Aug 2024 16:52:32 -0700 Subject: [PATCH 5/8] %S Created using spr 1.3.4 --- .../test/asan/TestCases/Linux/initialization-nobug-lld.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp index 80b502cd5538a..5595f9edce59b 100644 --- a/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp +++ b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp @@ -1,4 +1,4 @@ -// RUN: %clangxx_asan -O3 %p/initialization-nobug.cpp %S/Helpers/initialization-nobug-extra.cpp -fuse-ld=lld -o %t && %env_asan_opts=check_initialization_order=true:report_globals=3 %run %t 2>&1 | FileCheck %s --implicit-check-not "DynInit" +// RUN: %clangxx_asan -O3 %S/../initialization-nobug.cpp %S/../Helpers/initialization-nobug-extra.cpp -fuse-ld=lld -o %t && %env_asan_opts=check_initialization_order=true:report_globals=3 %run %t 2>&1 | FileCheck %s --implicit-check-not "DynInit" // Same as initialization-nobug.cpp, but with lld we expect just one // `DynInitUnpoison` executed after `AfterDynamicInit` at the end. From f70a22969ed233814a1398c3bfc86025ec06e9f6 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 7 Aug 2024 16:56:35 -0700 Subject: [PATCH 6/8] comment Created using spr 1.3.4 --- compiler-rt/lib/asan/asan_globals.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler-rt/lib/asan/asan_globals.cpp b/compiler-rt/lib/asan/asan_globals.cpp index 284fc8d2650f9..740b2a898f5b4 100644 --- a/compiler-rt/lib/asan/asan_globals.cpp +++ b/compiler-rt/lib/asan/asan_globals.cpp @@ -551,9 +551,7 @@ static void UnpoisonBeforeMain(void) { __attribute__((section(".init_array.65537"), used)) static void ( *asan_after_init_array)(void) = UnpoisonBeforeMain; #else -// Allow all `__asan_after_dynamic_init` if `AfterDynamicInit` is not set. -// Compiler still generates `__asan_{before,after}_dynamic_init`in pairs, and -// it's guaranteed that `__asan_after_dynamic_init` will be the last. +// Incremental poisoning is disabled, unpoison globals immediately. static constexpr bool allow_after_dynamic_init = true; #endif // SANITIZER_CAN_USE_PREINIT_ARRAY From 71f0fdc64b20c82ab3714e9bf2357a249126699d Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 7 Aug 2024 17:01:03 -0700 Subject: [PATCH 7/8] comment Created using spr 1.3.4 --- compiler-rt/lib/asan/asan_globals.cpp | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/compiler-rt/lib/asan/asan_globals.cpp b/compiler-rt/lib/asan/asan_globals.cpp index 740b2a898f5b4..c83b782cb85f8 100644 --- a/compiler-rt/lib/asan/asan_globals.cpp +++ b/compiler-rt/lib/asan/asan_globals.cpp @@ -521,15 +521,21 @@ void __asan_before_dynamic_init(const char *module_name) { } // Maybe SANITIZER_CAN_USE_PREINIT_ARRAY is to conservative for `.init_array`, -// however we should not make mistake here. If `AfterDynamicInit` was not +// however we should not make mistake here. If `UnpoisonBeforeMain` was not // executed at all we will have false reports on globals. #if SANITIZER_CAN_USE_PREINIT_ARRAY -// This is optimization. We will ignore all `__asan_after_dynamic_init`, but the -// last `__asan_after_dynamic_init`. We don't need number of -// `__asan_{before,after}_dynamic_init` matches, but we need that the last call -// was to `__asan_after_dynamic_init`, as it will unpoison all global preparing -// program for `main` execution. To run `__asan_after_dynamic_init` later, we -// will register in `.init_array`. +// This optimization aims to reduce the overhead of `__asan_after_dynamic_init` +// calls by leveraging incremental unpoisoning/poisoning in +// `__asan_before_dynamic_init`. We expect most `__asan_after_dynamic_init +// calls` to be no-ops. However, to ensure all globals are unpoisoned before the +// `main`, we force `UnpoisonBeforeMain` to fully execute +// `__asan_after_dynamic_init`. + +// With lld, `UnpoisonBeforeMain` runs after standard `.init_array`, making it +// the final `__asan_after_dynamic_init` call for the static runtime. In +// contrast, GNU ld executes it earlier, causing subsequent +// `__asan_after_dynamic_init` calls to perform full unpoisoning, losing the +// optimization. bool allow_after_dynamic_init SANITIZER_GUARDED_BY(mu_for_globals) = false; static void UnpoisonBeforeMain(void) { @@ -540,14 +546,10 @@ static void UnpoisonBeforeMain(void) { allow_after_dynamic_init = true; } if (flags()->report_globals >= 3) - Printf("AfterDynamicInit\n"); + Printf("UnpoisonBeforeMain\n"); __asan_after_dynamic_init(); } -// 65537 will make it run after constructors with default priority, but it -// requires ld.lld. With ld.bfd it can be called to early, and fail the -// optimization. However, correctness should not be affected, as after the first -// call all subsequent `__asan_after_dynamic_init` will be allowed. __attribute__((section(".init_array.65537"), used)) static void ( *asan_after_init_array)(void) = UnpoisonBeforeMain; #else From b7821ce93095cc62e889824dfb745839259aab7f Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 7 Aug 2024 23:06:50 -0700 Subject: [PATCH 8/8] update test for renames Created using spr 1.3.4 --- .../test/asan/TestCases/Linux/initialization-nobug-lld.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp index 5595f9edce59b..5cec029811cbc 100644 --- a/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp +++ b/compiler-rt/test/asan/TestCases/Linux/initialization-nobug-lld.cpp @@ -8,7 +8,7 @@ // contructors, with constructors of dynamic runtime. // XFAIL: asan-dynamic-runtime -// CHECK: DynInitPoison module: {{.*}}initialization-nobug.cpp -// CHECK: DynInitPoison module: {{.*}}initialization-nobug-extra.cpp -// CHECK: AfterDynamicInit +// CHECK: DynInitPoison +// CHECK: DynInitPoison +// CHECK: UnpoisonBeforeMain // CHECK: DynInitUnpoison