-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[asan][Windows] Synchronizing ASAN init on Windows #73824
[asan][Windows] Synchronizing ASAN init on Windows #73824
Conversation
Created using spr 1.3.4
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Zack Johnson (zacklj89) ChangesCreated using spr 1.3.4 Full diff: https://github.com/llvm/llvm-project/pull/73824.diff 3 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_internal.h b/compiler-rt/lib/asan/asan_internal.h
index e2b1e9800f5be62..f98cbd5da96bc8c 100644
--- a/compiler-rt/lib/asan/asan_internal.h
+++ b/compiler-rt/lib/asan/asan_internal.h
@@ -130,6 +130,19 @@ void InstallAtExitCheckLeaks();
if (&__asan_on_error) \
__asan_on_error()
+// Depending on the loading thread and when ASAN is loaded on Windows,
+// race conditions can appear causing incorrect states or internal check
+// failures.
+//
+// From a multithreaded managed environment, if an ASAN instrumented dll
+// is loading on a spawned thread, an intercepted function may be called on
+// multiple threads while ASAN is still in the process of initialization. This
+// can also cause the ASAN thread registry to create the "main" thread after
+// another thread, resulting in a TID != 0.
+//
+// Two threads can also race to initialize ASAN, resulting in either incorrect
+// state or internal check failures for init already running.
+//
bool AsanInited();
bool AsanInitIsRunning(); // Used to avoid infinite recursion in __asan_init().
extern bool replace_intrin_cached;
diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index d1e7856973b43b3..092ddf15d5a3db2 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -71,16 +71,54 @@ static void CheckUnwind() {
}
// -------------------------- Globals --------------------- {{{1
-static int asan_inited = 0;
-static int asan_init_is_running = 0;
+#if SANITIZER_WINDOWS
+atomic_uint8_t asan_inited{0};
+atomic_uint8_t asan_init_is_running{0};
+#else
+int asan_inited = 0;
+int asan_init_is_running = 0;
+#endif
-void SetAsanInited(u32 val) { asan_inited = val; }
+void SetAsanInited(u32 val) {
+#if SANITIZER_WINDOWS
+ atomic_store(&asan_inited, val, memory_order_release);
+#else
+ asan_inited = val;
+#endif
+}
-void SetAsanInitIsRunning(u32 val) { asan_init_is_running = val; }
+void SetAsanInitIsRunning(u32 val) {
+#if SANITIZER_WINDOWS
+ atomic_store(&asan_init_is_running, val, memory_order_release);
+#else
+ asan_init_is_running = val;
+#endif
+}
-bool AsanInited() { return asan_inited == 1; }
+bool AsanInited() {
+#if SANITIZER_WINDOWS
+ return atomic_load(&asan_inited, memory_order_acquire) == 1;
+#else
+ return asan_inited == 1;
+#endif
+}
-bool AsanInitIsRunning() { return asan_init_is_running == 1; }
+bool AsanInitIsRunning() {
+#if SANITIZER_WINDOWS
+ return atomic_load(&asan_init_is_running, memory_order_acquire) == 1;
+#else
+ return asan_init_is_running == 1;
+#endif
+}
+
+void CheckAsanInitRunning() {
+#if SANITIZER_WINDOWS
+ while (AsanInitIsRunning()) {
+ // If ASAN is initializing on another thread, wait for it to finish.
+ internal_sched_yield();
+ }
+#endif
+}
bool replace_intrin_cached;
@@ -391,6 +429,7 @@ void PrintAddressSpaceLayout() {
}
static void AsanInitInternal() {
+ CheckAsanInitRunning();
if (LIKELY(AsanInited()))
return;
SanitizerToolName = "AddressSanitizer";
diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp
index 8798968947e82e6..dc0ad2caf3bbd10 100644
--- a/compiler-rt/lib/asan/asan_thread.cpp
+++ b/compiler-rt/lib/asan/asan_thread.cpp
@@ -27,6 +27,10 @@ namespace __asan {
// AsanThreadContext implementation.
+#if SANITIZER_WINDOWS
+static atomic_uint8_t main_thread_created{0};
+#endif
+
void AsanThreadContext::OnCreated(void *arg) {
CreateThreadContextArgs *args = static_cast<CreateThreadContextArgs *>(arg);
if (args->stack)
@@ -93,6 +97,12 @@ AsanThreadContext *GetThreadContextByTidLocked(u32 tid) {
AsanThread *AsanThread::Create(const void *start_data, uptr data_size,
u32 parent_tid, StackTrace *stack,
bool detached) {
+#if SANITIZER_WINDOWS
+ while (atomic_load(&main_thread_created, memory_order_acquire) == 0) {
+ // If another thread is trying to be created before the main thread, wait.
+ internal_sched_yield();
+ }
+#endif
uptr PageSize = GetPageSizeCached();
uptr size = RoundUpTo(sizeof(AsanThread), PageSize);
AsanThread *thread = (AsanThread *)MmapOrDie(size, __func__);
@@ -288,11 +298,25 @@ void AsanThread::ThreadStart(tid_t os_id) {
}
AsanThread *CreateMainThread() {
+// Depending on the loading thread, specifically in managed scenarios, the main
+// thread can be created after other threads on Windows. This ensures we start
+// the main thread before those threads.
+# if SANITIZER_WINDOWS
+ uptr PageSize = GetPageSizeCached();
+ uptr size = RoundUpTo(sizeof(AsanThread), PageSize);
+ AsanThread *main_thread = (AsanThread *)MmapOrDie(size, __func__);
+ AsanThreadContext::CreateThreadContextArgs args = {main_thread, nullptr};
+ asanThreadRegistry().CreateThread(0, true, kMainTid, &args);
+ SetCurrentThread(main_thread);
+ main_thread->ThreadStart(internal_getpid());
+ atomic_store(&main_thread_created, 1, memory_order_release);
+# else
AsanThread *main_thread = AsanThread::Create(
/* parent_tid */ kMainTid,
/* stack */ nullptr, /* detached */ true);
SetCurrentThread(main_thread);
main_thread->ThreadStart(internal_getpid());
+# endif
return main_thread;
}
|
// Depending on the loading thread and when ASAN is loaded on Windows, | ||
// race conditions can appear causing incorrect states or internal check | ||
// failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still true with changes from that patch?
Also, it would be nice, if explanation from that comment would be in a PR description and commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I'm trying to fulfill the repo's PR requirements here: #71833 (comment)
This is just one of the spr PRs created. It appears I've done it incorrectly, though. I'll be closing this PR.
Created using spr 1.3.4