Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ASan][Windows] Synchronizing ASAN init on Windows #71833

Closed
wants to merge 2 commits into from
Closed

[ASan][Windows] Synchronizing ASAN init on Windows #71833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd2beca
[asan] Fixing initialization synchronization on Windows
zacklj89 Nov 9, 2023
45b80b9
adding yields
zacklj89 Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions compiler-rt/lib/asan/asan_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,19 @@ void InstallAtForkHandler();
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();
extern bool replace_intrin_cached;
extern void (*death_callback)(void);
Expand Down
24 changes: 24 additions & 0 deletions compiler-rt/lib/asan/asan_thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how newthread can happen? Isn't there is interceptor for CreateThread which should be executed first on main thread?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like dead lock prone solution
Whoever is creator of this thread may wait to join this what, and prevent initialization happening

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitalybuka, do the changes look sufficient to you after the latest push?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that, I expected it would be easier.
We can go one by one with regular PRs.

Copy link
Contributor Author

@zacklj89 zacklj89 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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__);
Expand Down Expand Up @@ -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;
}

Expand Down