[asan] Switch initialization to "double-checked locking"

[vitalybuka]

This allows to remove asan_init_is_running which likely had a data
race.

Simplifies #74086 and reduces a difference between platforms.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Vitaly Buka (vitalybuka)

Changes

This allows to remove asan_init_is_running which likely had a data
race.

---

Full diff: https://github.com/llvm/llvm-project/pull/74387.diff

1 Files Affected:

• (modified) compiler-rt/lib/asan/asan_rtl.cpp (+25-24)

diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index 04ecd20821fa6..52d3461ada6c6 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -71,16 +71,16 @@ static void CheckUnwind() {
 }
 
 // -------------------------- Globals --------------------- {{{1
-static int asan_inited = 0;
-static int asan_init_is_running = 0;
+static StaticSpinMutex asan_inited_mutex;
+static atomic_uint8_t asan_inited = {0};
 
-static void SetAsanInited() { asan_inited = 1; }
-
-static void SetAsanInitIsRunning(u32 val) { asan_init_is_running = val; }
-
-bool AsanInited() { return asan_inited == 1; }
+static void SetAsanInited() {
+  atomic_store(&asan_inited, 1, memory_order_release);
+}
 
-static bool AsanInitIsRunning() { return asan_init_is_running == 1; }
+bool AsanInited() {
+  return atomic_load(&asan_inited, memory_order_acquire) == 1;
+}
 
 bool replace_intrin_cached;
 
@@ -390,12 +390,10 @@ void PrintAddressSpaceLayout() {
           kHighShadowBeg > kMidMemEnd);
 }
 
-static void AsanInitInternal() {
+static bool AsanInitInternal() {
   if (LIKELY(AsanInited()))
-    return;
+    return true;
   SanitizerToolName = "AddressSanitizer";
-  CHECK(!AsanInitIsRunning() && "ASan init calls itself!");
-  SetAsanInitIsRunning(1);
 
   CacheBinaryName();
 
@@ -408,9 +406,8 @@ static void AsanInitInternal() {
   // Stop performing init at this point if we are being loaded via
   // dlopen() and the platform supports it.
   if (SANITIZER_SUPPORTS_INIT_FOR_DLOPEN && UNLIKELY(HandleDlopenInit())) {
-    SetAsanInitIsRunning(0);
     VReport(1, "AddressSanitizer init is being performed for dlopen().\n");
-    return;
+    return false;
   }
 
   AsanCheckIncompatibleRT();
@@ -471,7 +468,6 @@ static void AsanInitInternal() {
   // should be set to 1 prior to initializing the threads.
   replace_intrin_cached = flags()->replace_intrin;
   SetAsanInited();
-  SetAsanInitIsRunning(0);
 
   if (flags()->atexit)
     Atexit(asan_atexit);
@@ -515,22 +511,27 @@ static void AsanInitInternal() {
   VReport(1, "AddressSanitizer Init done\n");
 
   WaitForDebugger(flags()->sleep_after_init, "after init");
+
+  return true;
 }
 
 // Initialize as requested from some part of ASan runtime library (interceptors,
 // allocator, etc).
 void AsanInitFromRtl() {
-  CHECK(!AsanInitIsRunning());
-  if (UNLIKELY(!AsanInited()))
-    AsanInitInternal();
+  if (LIKELY(AsanInited()))
+    return;
+  SpinMutexLock lock(&asan_inited_mutex);
+  AsanInitInternal();
 }
 
 bool TryAsanInitFromRtl() {
-  if (UNLIKELY(AsanInitIsRunning()))
+  if (LIKELY(AsanInited()))
+    return true;
+  if (!asan_inited_mutex.TryLock())
     return false;
-  if (UNLIKELY(!AsanInited()))
-    AsanInitInternal();
-  return true;
+  bool result = AsanInitInternal();
+  asan_inited_mutex.Unlock();
+  return result;
 }
 
 #if ASAN_DYNAMIC
@@ -603,7 +604,7 @@ static void UnpoisonFakeStack() {
 using namespace __asan;
 
 void NOINLINE __asan_handle_no_return() {
-  if (AsanInitIsRunning())
+  if (!TryAsanInitFromRtl())
     return;
 
   if (!PlatformUnpoisonStacks())
@@ -633,7 +634,7 @@ void NOINLINE __asan_set_death_callback(void (*callback)(void)) {
 // We use this call as a trigger to wake up ASan from deactivated state.
 void __asan_init() {
   AsanActivate();
-  AsanInitInternal();
+  AsanInitFromRtl();
 }
 
 void __asan_version_mismatch_check() {

[dvyukov]

If we return false here, __asan_handle_no_return won't unpoison stack.
It looks like it's different from the current behavior.
What's the reason for the change?

[vitalybuka]

We didn't get to InitializeShadowMemory below, seems now it must do nothing or crash anyway.

[vitalybuka]

Maybe it's OK to block and wait in __asan_handle_no_return, but as-is seems close to the current behavior.

[vitalybuka]

I believe __asan_handle_no_return is unsupported before initialization, so

if (UNLIKELY(!AsanInited()))
    return;

should be as good as the current state.

[vitalybuka]

To clarify

Before the patch

• asan_inited = 0, asan_init_is_running = 0: __asan_handle_no_return proceeds but likely going to crash, e.g. on llvm-project/compiler-rt/lib/asan/asan_poisoning.cpp:41
  So we don't see this case in practice.

• asan_inited = 0, asan_init_is_running = 1: __asan_handle_no_return returns doing nothing

• asan_inited = 1, asan_init_is_running = 0: __asan_handle_no_return works as expected

After the patch:

• !AsanInited(): __asan_handle_no_return return doing noting

• AsanInited(): __asan_handle_no_return works as expected

[dvyukov]

This also seems to introduce a change in the behavior. Currently __asan_handle_no_return returns early only while AsanInitIsRunning, but the mutex lock duration covers whole AsanInitInternal.
Is this intentional?

[vitalybuka]

Thanks. No, unintentional. I missed this one.
I'll try to split them.

[vitalybuka]

Actually I don't believe we care about partially initialized asan for __asan_handle_no_return.
I think __asan_handle_no_return should bailout aggressively.

[zacklj89]

Does it make sense to wrap in CHECK?

[vitalybuka]

We will have to do something about SANITIZER_SUPPORTS_INIT_FOR_DLOPEN
I'd like to avoid dealing with that in this patch.

[vitalybuka]

@dvyukov @eugenis If no additional feedback, I will land it?

[dvyukov]

I don't have any objections.
There may be some subtle implications, or maybe not. I guess it's impossible to understand w/o merging this.
The increased critical section may lead to deadlocks on recursion, or not.