[ASan][Windows] Synchronizing ASAN init on Windows #71833
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Zack Johnson (zacklj89) ChangesThis is an attempt to synchronize ASAN initialization using This addresses:
Currently, everything is blocked by Full diff: https://github.com/llvm/llvm-project/pull/71833.diff 12 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_allocator.cpp b/compiler-rt/lib/asan/asan_allocator.cpp
index 15eefcb96063a88..22dcf6132707b52 100644
--- a/compiler-rt/lib/asan/asan_allocator.cpp
+++ b/compiler-rt/lib/asan/asan_allocator.cpp
@@ -527,7 +527,7 @@ struct Allocator {
// -------------------- Allocation/Deallocation routines ---------------
void *Allocate(uptr size, uptr alignment, BufferedStackTrace *stack,
AllocType alloc_type, bool can_fill) {
- if (UNLIKELY(!asan_inited))
+ if (UNLIKELY(!AsanInited()))
AsanInitFromRtl();
if (UNLIKELY(IsRssLimitExceeded())) {
if (AllocatorMayReturnNull())
diff --git a/compiler-rt/lib/asan/asan_globals.cpp b/compiler-rt/lib/asan/asan_globals.cpp
index 14c36bdc8e64296..597c710690dadb1 100644
--- a/compiler-rt/lib/asan/asan_globals.cpp
+++ b/compiler-rt/lib/asan/asan_globals.cpp
@@ -196,7 +196,7 @@ static inline bool UseODRIndicator(const Global *g) {
// This function may be called more than once for every global
// so we store the globals in a map.
static void RegisterGlobal(const Global *g) {
- CHECK(asan_inited);
+ CHECK(AsanInited());
if (flags()->report_globals >= 2)
ReportGlobal(*g, "Added");
CHECK(flags()->report_globals);
@@ -237,7 +237,7 @@ static void RegisterGlobal(const Global *g) {
}
static void UnregisterGlobal(const Global *g) {
- CHECK(asan_inited);
+ CHECK(AsanInited());
if (flags()->report_globals >= 2)
ReportGlobal(*g, "Removed");
CHECK(flags()->report_globals);
@@ -427,7 +427,7 @@ void __asan_before_dynamic_init(const char *module_name) {
return;
bool strict_init_order = flags()->strict_init_order;
CHECK(module_name);
- CHECK(asan_inited);
+ CHECK(AsanInited());
Lock lock(&mu_for_globals);
if (flags()->report_globals >= 3)
Printf("DynInitPoison module: %s\n", module_name);
@@ -451,7 +451,7 @@ void __asan_after_dynamic_init() {
!CanPoisonMemory() ||
!dynamic_init_globals)
return;
- CHECK(asan_inited);
+ CHECK(AsanInited());
Lock lock(&mu_for_globals);
// FIXME: Optionally report that we're unpoisoning globals from a module.
for (uptr i = 0, n = dynamic_init_globals->size(); i < n; ++i) {
diff --git a/compiler-rt/lib/asan/asan_interceptors.cpp b/compiler-rt/lib/asan/asan_interceptors.cpp
index 0ee8e0ea786edeb..80d00a728100d1f 100644
--- a/compiler-rt/lib/asan/asan_interceptors.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors.cpp
@@ -99,9 +99,9 @@ DECLARE_REAL_AND_INTERCEPTOR(void, free, void *)
#define COMMON_INTERCEPTOR_ENTER(ctx, func, ...) \
ASAN_INTERCEPTOR_ENTER(ctx, func); \
do { \
- if (asan_init_is_running) \
+ if (AsanInitIsRunning()) \
return REAL(func)(__VA_ARGS__); \
- if (SANITIZER_APPLE && UNLIKELY(!asan_inited)) \
+ if (SANITIZER_APPLE && UNLIKELY(!AsanInited())) \
return REAL(func)(__VA_ARGS__); \
ENSURE_ASAN_INITED(); \
} while (false)
@@ -138,7 +138,7 @@ DECLARE_REAL_AND_INTERCEPTOR(void, free, void *)
# define COMMON_INTERCEPTOR_ON_EXIT(ctx) OnExit()
# define COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, handle)
# define COMMON_INTERCEPTOR_LIBRARY_UNLOADED()
-# define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (!asan_inited)
+# define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (!AsanInited())
# define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end) \
if (AsanThread *t = GetCurrentThread()) { \
*begin = t->tls_begin(); \
@@ -535,12 +535,12 @@ INTERCEPTOR(char *, strcpy, char *to, const char *from) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, strcpy);
#if SANITIZER_APPLE
- if (UNLIKELY(!asan_inited))
+ if (UNLIKELY(!AsanInited()))
return REAL(strcpy)(to, from);
#endif
// strcpy is called from malloc_default_purgeable_zone()
// in __asan::ReplaceSystemAlloc() on Mac.
- if (asan_init_is_running) {
+ if (AsanInitIsRunning()) {
return REAL(strcpy)(to, from);
}
ENSURE_ASAN_INITED();
@@ -556,7 +556,7 @@ INTERCEPTOR(char *, strcpy, char *to, const char *from) {
INTERCEPTOR(char*, strdup, const char *s) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, strdup);
- if (UNLIKELY(!asan_inited)) return internal_strdup(s);
+ if (UNLIKELY(!AsanInited())) return internal_strdup(s);
ENSURE_ASAN_INITED();
uptr length = internal_strlen(s);
if (flags()->replace_str) {
@@ -574,7 +574,7 @@ INTERCEPTOR(char*, strdup, const char *s) {
INTERCEPTOR(char*, __strdup, const char *s) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, strdup);
- if (UNLIKELY(!asan_inited)) return internal_strdup(s);
+ if (UNLIKELY(!AsanInited())) return internal_strdup(s);
ENSURE_ASAN_INITED();
uptr length = internal_strlen(s);
if (flags()->replace_str) {
@@ -634,7 +634,7 @@ INTERCEPTOR(int, atoi, const char *nptr) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, atoi);
#if SANITIZER_APPLE
- if (UNLIKELY(!asan_inited)) return REAL(atoi)(nptr);
+ if (UNLIKELY(!AsanInited())) return REAL(atoi)(nptr);
#endif
ENSURE_ASAN_INITED();
if (!flags()->replace_str) {
@@ -655,7 +655,7 @@ INTERCEPTOR(long, atol, const char *nptr) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, atol);
#if SANITIZER_APPLE
- if (UNLIKELY(!asan_inited)) return REAL(atol)(nptr);
+ if (UNLIKELY(!AsanInited())) return REAL(atol)(nptr);
#endif
ENSURE_ASAN_INITED();
if (!flags()->replace_str) {
@@ -693,7 +693,7 @@ static void AtCxaAtexit(void *unused) {
INTERCEPTOR(int, __cxa_atexit, void (*func)(void *), void *arg,
void *dso_handle) {
#if SANITIZER_APPLE
- if (UNLIKELY(!asan_inited)) return REAL(__cxa_atexit)(func, arg, dso_handle);
+ if (UNLIKELY(!AsanInited())) return REAL(__cxa_atexit)(func, arg, dso_handle);
#endif
ENSURE_ASAN_INITED();
#if CAN_SANITIZE_LEAKS
diff --git a/compiler-rt/lib/asan/asan_interceptors.h b/compiler-rt/lib/asan/asan_interceptors.h
index d00d05587b36881..19a40f18c43fa42 100644
--- a/compiler-rt/lib/asan/asan_interceptors.h
+++ b/compiler-rt/lib/asan/asan_interceptors.h
@@ -26,8 +26,8 @@ void InitializePlatformInterceptors();
#define ENSURE_ASAN_INITED() \
do { \
- CHECK(!asan_init_is_running); \
- if (UNLIKELY(!asan_inited)) { \
+ CHECK(!AsanInitIsRunning()); \
+ if (UNLIKELY(!AsanInited())) {\
AsanInitFromRtl(); \
} \
} while (0)
diff --git a/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp b/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
index 4e4ea7191d3204f..bdf328f8920634d 100644
--- a/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
@@ -33,7 +33,7 @@ using namespace __asan;
} \
ASAN_READ_RANGE(ctx, from, size); \
ASAN_WRITE_RANGE(ctx, to, size); \
- } else if (UNLIKELY(!asan_inited)) { \
+ } else if (UNLIKELY(!AsanInited())) { \
return internal_memcpy(to, from, size); \
} \
return REAL(memcpy)(to, from, size); \
@@ -44,7 +44,7 @@ using namespace __asan;
do { \
if (LIKELY(replace_intrin_cached)) { \
ASAN_WRITE_RANGE(ctx, block, size); \
- } else if (UNLIKELY(!asan_inited)) { \
+ } else if (UNLIKELY(!AsanInited())) { \
return internal_memset(block, c, size); \
} \
return REAL(memset)(block, c, size); \
diff --git a/compiler-rt/lib/asan/asan_internal.h b/compiler-rt/lib/asan/asan_internal.h
index a5348e35b297eb1..b8b8b44d0dcbb3e 100644
--- a/compiler-rt/lib/asan/asan_internal.h
+++ b/compiler-rt/lib/asan/asan_internal.h
@@ -130,9 +130,22 @@ void InstallAtExitCheckLeaks();
if (&__asan_on_error) \
__asan_on_error()
-extern int asan_inited;
-// Used to avoid infinite recursion in __asan_init().
-extern bool asan_init_is_running;
+// 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;
extern void (*death_callback)(void);
// These magic values are written to shadow for better error
diff --git a/compiler-rt/lib/asan/asan_malloc_linux.cpp b/compiler-rt/lib/asan/asan_malloc_linux.cpp
index bab80b96f584b90..0ba74c5d71432bd 100644
--- a/compiler-rt/lib/asan/asan_malloc_linux.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_linux.cpp
@@ -31,7 +31,7 @@
using namespace __asan;
struct DlsymAlloc : public DlSymAllocator<DlsymAlloc> {
- static bool UseImpl() { return asan_init_is_running; }
+ static bool UseImpl() { return AsanInitIsRunning(); }
static void OnAllocate(const void *ptr, uptr size) {
# if CAN_SANITIZE_LEAKS
// Suppress leaks from dlerror(). Previously dlsym hack on global array was
diff --git a/compiler-rt/lib/asan/asan_malloc_mac.cpp b/compiler-rt/lib/asan/asan_malloc_mac.cpp
index 924d1f12640a7a9..242f030b000a299 100644
--- a/compiler-rt/lib/asan/asan_malloc_mac.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_mac.cpp
@@ -23,7 +23,7 @@
using namespace __asan;
#define COMMON_MALLOC_ZONE_NAME "asan"
#define COMMON_MALLOC_ENTER() ENSURE_ASAN_INITED()
-#define COMMON_MALLOC_SANITIZER_INITIALIZED asan_inited
+#define COMMON_MALLOC_SANITIZER_INITIALIZED AsanInited()
#define COMMON_MALLOC_FORCE_LOCK() asan_mz_force_lock()
#define COMMON_MALLOC_FORCE_UNLOCK() asan_mz_force_unlock()
#define COMMON_MALLOC_MEMALIGN(alignment, size) \
diff --git a/compiler-rt/lib/asan/asan_malloc_win.cpp b/compiler-rt/lib/asan/asan_malloc_win.cpp
index ff78d7646a90dc8..7e1d04c36dd5801 100644
--- a/compiler-rt/lib/asan/asan_malloc_win.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_win.cpp
@@ -211,7 +211,7 @@ INTERCEPTOR_WINAPI(size_t, HeapSize, HANDLE hHeap, DWORD dwFlags,
// interception takes place, so if it is not owned by the RTL heap we can
// pass it to the ASAN heap for inspection.
if (flags()->windows_hook_rtl_allocators) {
- if (!asan_inited || OWNED_BY_RTL(hHeap, lpMem))
+ if (!AsanInited() || OWNED_BY_RTL(hHeap, lpMem))
return REAL(HeapSize)(hHeap, dwFlags, lpMem);
} else {
CHECK(dwFlags == 0 && "unsupported heap flags");
@@ -226,7 +226,7 @@ INTERCEPTOR_WINAPI(LPVOID, HeapAlloc, HANDLE hHeap, DWORD dwFlags,
// If the ASAN runtime is not initialized, or we encounter an unsupported
// flag, fall back to the original allocator.
if (flags()->windows_hook_rtl_allocators) {
- if (UNLIKELY(!asan_inited ||
+ if (UNLIKELY(!AsanInited() ||
(dwFlags & HEAP_ALLOCATE_UNSUPPORTED_FLAGS) != 0)) {
return REAL(HeapAlloc)(hHeap, dwFlags, dwBytes);
}
@@ -297,7 +297,7 @@ void *SharedReAlloc(ReAllocFunction reallocFunc, SizeFunction heapSizeFunc,
// If this heap block which was allocated before the ASAN
// runtime came up, use the real HeapFree function.
- if (UNLIKELY(!asan_inited)) {
+ if (UNLIKELY(!AsanInited())) {
return reallocFunc(hHeap, dwFlags, lpMem, dwBytes);
}
bool only_asan_supported_flags =
@@ -420,7 +420,7 @@ size_t RtlSizeHeap(void* HeapHandle, DWORD Flags, void* BaseAddress);
INTERCEPTOR_WINAPI(size_t, RtlSizeHeap, HANDLE HeapHandle, DWORD Flags,
void* BaseAddress) {
if (!flags()->windows_hook_rtl_allocators ||
- UNLIKELY(!asan_inited || OWNED_BY_RTL(HeapHandle, BaseAddress))) {
+ UNLIKELY(!AsanInited() || OWNED_BY_RTL(HeapHandle, BaseAddress))) {
return REAL(RtlSizeHeap)(HeapHandle, Flags, BaseAddress);
}
GET_CURRENT_PC_BP_SP;
@@ -448,7 +448,7 @@ INTERCEPTOR_WINAPI(void*, RtlAllocateHeap, HANDLE HeapHandle, DWORD Flags,
// If the ASAN runtime is not initialized, or we encounter an unsupported
// flag, fall back to the original allocator.
if (!flags()->windows_hook_rtl_allocators ||
- UNLIKELY(!asan_inited ||
+ UNLIKELY(!AsanInited() ||
(Flags & HEAP_ALLOCATE_UNSUPPORTED_FLAGS) != 0)) {
return REAL(RtlAllocateHeap)(HeapHandle, Flags, Size);
}
diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index 853083182b48787..2cdaf88278455b0 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -71,8 +71,55 @@ static void CheckUnwind() {
}
// -------------------------- Globals --------------------- {{{1
-int asan_inited;
-bool asan_init_is_running;
+#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) {
+#if SANITIZER_WINDOWS
+ atomic_store(&asan_inited, val, memory_order_release);
+#else
+ asan_inited = val;
+#endif
+}
+
+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() {
+#if SANITIZER_WINDOWS
+ return atomic_load(&asan_inited, memory_order_acquire) == 1;
+#else
+ return asan_inited == 1;
+#endif
+}
+
+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.
+ }
+#endif
+ return;
+}
+
bool replace_intrin_cached;
#if !ASAN_FIXED_MAPPING
@@ -382,10 +429,11 @@ void PrintAddressSpaceLayout() {
}
static void AsanInitInternal() {
- if (LIKELY(asan_inited)) return;
+ CheckAsanInitRunning();
+ if (LIKELY(AsanInited())) return;
SanitizerToolName = "AddressSanitizer";
- CHECK(!asan_init_is_running && "ASan init calls itself!");
- asan_init_is_running = true;
+ CHECK(!AsanInitIsRunning() && "ASan init calls itself!");
+ SetAsanInitIsRunning(1);
CacheBinaryName();
@@ -398,7 +446,7 @@ 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())) {
- asan_init_is_running = false;
+ SetAsanInitIsRunning(0);
VReport(1, "AddressSanitizer init is being performed for dlopen().\n");
return;
}
@@ -460,8 +508,8 @@ static void AsanInitInternal() {
// On Linux AsanThread::ThreadStart() calls malloc() that's why asan_inited
// should be set to 1 prior to initializing the threads.
replace_intrin_cached = flags()->replace_intrin;
- asan_inited = 1;
- asan_init_is_running = false;
+ SetAsanInited(1);
+ SetAsanInitIsRunning(0);
if (flags()->atexit)
Atexit(asan_atexit);
@@ -583,7 +631,7 @@ static void UnpoisonFakeStack() {
using namespace __asan;
void NOINLINE __asan_handle_no_return() {
- if (asan_init_is_running)
+ if (AsanInitIsRunning())
return;
if (!PlatformUnpoisonStacks())
diff --git a/compiler-rt/lib/asan/asan_stack.cpp b/compiler-rt/lib/asan/asan_stack.cpp
index 048295d6928ad5d..764c6ac193fb063 100644
--- a/compiler-rt/lib/asan/asan_stack.cpp
+++ b/compiler-rt/lib/asan/asan_stack.cpp
@@ -57,7 +57,7 @@ void __sanitizer::BufferedStackTrace::UnwindImpl(
uptr pc, uptr bp, void *context, bool request_fast, u32 max_depth) {
using namespace __asan;
size = 0;
- if (UNLIKELY(!asan_inited))
+ if (UNLIKELY(!AsanInited()))
return;
request_fast = StackTrace::WillUseFastUnwind(request_fast);
AsanThread *t = GetCurrentThread();
diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp
index 8798968947e82e6..4f70731a329c341 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
+atomic_uint8_t mainThreadCreated{0};
+#endif
+
void AsanThreadContext::OnCreated(void *arg) {
CreateThreadContextArgs *args = static_cast<CreateThreadContextArgs *>(arg);
if (args->stack)
@@ -93,6 +97,11 @@ 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(&mainThreadCreated, memory_order_acquire) == 0) {
+ // If another thread is trying to be created before the main thread, wait.
+ }
+#endif
uptr PageSize = GetPageSizeCached();
uptr size = RoundUpTo(sizeof(AsanThread), PageSize);
AsanThread *thread = (AsanThread *)MmapOrDie(size, __func__);
@@ -288,11 +297,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(&mainThreadCreated, 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;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
zacklj89
force-pushed
the
zacklj89/asan-init-windows-synchronization
branch
from
9a50ce7
to
baa2227
Compare
vitalybuka
requested changes
Nov 9, 2023
There was a problem hiding this comment.
We like small commits here. Unfortunately github is not very suitable, or LLVM didn't figured our what is going to be a flow.
Still I will ask you extract into NFC patches:
- NFC: All clang-format changes
- NFC: Introduction of setters/getter, as-is without atomics
- Real patch with windows synchronization
|
Thanks @vitalybuka! I was trying to add reviewers as well (you, @strega-nil , and @barcharcraz), but I was unable to. Are there specific permissions needed, or something I missed in the Contributing.md? |
zacklj89
force-pushed
the
zacklj89/asan-init-windows-synchronization
branch
from
baa2227
to
85992f6
Compare
zacklj89
changed the title
I mean separate pull request, so we can bisect issues if necessary. |
vitalybuka
reviewed
Nov 9, 2023
vitalybuka
force-pushed
the
zacklj89/asan-init-windows-synchronization
branch
from
85992f6
to
cf1f329
Compare
vitalybuka
pushed a commit
that referenced
this pull request
Nov 9, 2023
vitalybuka
pushed a commit
that referenced
this pull request
Nov 9, 2023
vitalybuka
force-pushed
the
zacklj89/asan-init-windows-synchronization
branch
2 times, most recently
from
0904beb
to
441bebd
Compare
vitalybuka
reviewed
Nov 9, 2023
compiler-rt/lib/asan/asan_rtl.cpp
Outdated
| @@ -71,16 +71,54 @@ static void CheckUnwind() { | |||
| } | |||
|
|
|||
| // -------------------------- Globals --------------------- {{{1 | |||
| static int asan_inited = 0; | |||
| static int asan_init_is_running = 0; | |||
| #if SANITIZER_WINDOWS | |||
There was a problem hiding this comment.
do we need to #if ?
maybe atomic everywhere?
There was a problem hiding this comment.
I wasn't sure how the change would be viewed, so I added this for Windows only. If it seems benign, I'll remove the #if SANITIZER_WINDOWS
vitalybuka
reviewed
Nov 9, 2023
compiler-rt/lib/asan/asan_rtl.cpp
Outdated
| void CheckAsanInitRunning() { | ||
| #if SANITIZER_WINDOWS | ||
| while (AsanInitIsRunning()) { | ||
| // If ASAN is initializing on another thread, wait for it to finish. |
There was a problem hiding this comment.
do we need some kind of yeld here?
There was a problem hiding this comment.
Good point. I was hesitant to add yields or mutex usage, but I think yield makes sense here.
vitalybuka
reviewed
Nov 9, 2023
| @@ -93,6 +97,11 @@ AsanThreadContext *GetThreadContextByTidLocked(u32 tid) { | |||
| AsanThread *AsanThread::Create(const void *start_data, uptr data_size, | |||
| u32 parent_tid, StackTrace *stack, | |||
| bool detached) { | |||
| #if SANITIZER_WINDOWS | |||
There was a problem hiding this comment.
how newthread can happen? Isn't there is interceptor for CreateThread which should be executed first on main thread?
There was a problem hiding this comment.
Looks like dead lock prone solution
Whoever is creator of this thread may wait to join this what, and prevent initialization happening
There was a problem hiding this comment.
From .NET, if a worker thread loads an ASAN instrumented binary, we can see all of the checks in the PR description fail based on timing.
For the check ((!asan_init_is_running && "ASan init calls itself!")) != (0), this can happen when one thread is initializing ASan and has already intercepted malloc or Rtl* APIs. While it's still initializing, another thread calls the intercepted API, which results in the __asan::Allocator making another call to AsanInitInternal, and the check fires.
For the main thread creation, this happens less frequently since the call to CreateMainThread is nearly at the end of AsanInitInternal shortly after CreateThread has been intercepted. After it's intercepted, the thread registry can attempt to create another thread before the call to create the main thread. I'm not sure how ASan can be dynamically loaded on other platforms besides Windows, so this might not be an issue outside of Windows.
There was a problem hiding this comment.
@vitalybuka, do the changes look sufficient to you after the latest push?
There was a problem hiding this comment.
You replied as "will do" to some items, as #if
But I see update in this PR.
Also there are zahiraam@8e9ce62
you want to create PR for them?
BTW. There is https://github.com/getcord/spr which is proposed to be used for stacked code review, similar to we have in Phabricator, you may try that.
There was a problem hiding this comment.
It's close, but I assume you have branch per PR locally, because PRs include diffs from other PRs
With SPR you can have all patches in local branch, and upload them with spr diff --all, then it will create stacked review similar we had with Phabricator.
There was a problem hiding this comment.
And SPR is suppose to upload into upstream repo, not your fork
llvm-project/.git/config should look like:
githubRemoteName = origin
githubRepository = llvm/llvm-project
githubMasterBranch = main
branchPrefix = users/vitalybuka/spr/
requireTestPlan = false
There was a problem hiding this comment.
I assumed I had to use my fork to create the PR since I don't have push permissions to llvm. I tried changing the configuration and received invalid PR.
I'll see if @barcharcraz can help me with this live today.
There was a problem hiding this comment.
Sorry for that, I expected it would be easier.
We can go one by one with regular PRs.
There was a problem hiding this comment.
I was able to get LLVM push permissions, and I created the PRs using spr diff --all on the main repo. Here is the last stacked PR. #74086
The other PRs are in the description.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
This was referenced Nov 29, 2023
vitalybuka
requested changes
Dec 18, 2023
zacklj89
force-pushed
the
zacklj89/asan-init-windows-synchronization
branch
from
683139e
to
45b80b9
Compare
|
Apologies @vitalybuka, just updated it. The comment on the other PR about moving to I will be out of office until the new year, so if this doesn't get merged I'll check back in then. Happy holidays! |
|
@vitalybuka @barcharcraz is there anything further needed here? |
|
I will close this for now since there has been no activity, and I believe @barcharcraz is in the process of making changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to synchronize ASAN initialization using
__sanitizer::atomics on Windows. One case in particular is a C# application that loads an ASan instrumented library.This addresses:
ENSURE_ASAN_INITED.CreateThreadis intercepted.Currently, everything is blocked by
#if SANITIZER_WINDOWSbecause I'm not 100% sure about how ASan is loaded on other platforms and the implications.