[asan][win] Fix CreateThread leak #126738

GkvJwa · 2025-02-11T14:45:05Z

Since t->Destroy cannot be called after start_routine(When calling standard thread_start in crt), the func is run in advance to avoid memory leaks and remain the same as before.

llvmbot · 2025-02-11T14:45:46Z

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

Author: None (GkvJwa)

Changes

Fix #126541

Since t->Destroy cannot be called after start_routine(When calling standard thread_start in crt), the func is run in advance to avoid memory leaks and remain the same as before.

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

1 Files Affected:

(modified) compiler-rt/lib/asan/asan_win.cpp (+3-1)

diff --git a/compiler-rt/lib/asan/asan_win.cpp b/compiler-rt/lib/asan/asan_win.cpp
index 09a13b11cff1f53..d043bb822d2cf15 100644
--- a/compiler-rt/lib/asan/asan_win.cpp
+++ b/compiler-rt/lib/asan/asan_win.cpp
@@ -143,9 +143,11 @@ static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
 
   ThreadStartParams params;
   t->GetStartData(params);
+  // The ExitThread will end the current thread, causing destroy to be unable to
+  // be called.
+  t->Destroy();
 
   auto res = (*params.start_routine)(params.arg);
-  t->Destroy();  // POSIX calls this from TSD destructor.
   return res;
 }

rnk · 2025-02-13T21:23:00Z

compiler-rt/lib/asan/asan_win.cpp

@@ -143,9 +143,11 @@ static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {

  ThreadStartParams params;
  t->GetStartData(params);
+  // The ExitThread will end the current thread, causing destroy to be unable to
+  // be called.
+  t->Destroy();


I think t is escaped above, so this isn't correct, it needs to happen after the thread runs.

If the user thread routine calls ExitThread manually rather than returning, then the proper fix for ASan is probably to intercept ExitThread and do the deallocation there.

Also, it sounds like small memory leaks are expected when calling ExitThread, so maybe we shouldn't worry about this:

A thread in an executable that is linked to the static C run-time library (CRT) should use _beginthread and _endthread for thread management rather than CreateThread and ExitThread. Failure to do so results in small memory leaks when the thread calls ExitThread.

I think t is escaped above, so this isn't correct, it needs to happen after the thread runs.

If the user thread routine calls ExitThread manually rather than returning, then the proper fix for ASan is probably to intercept ExitThread and do the deallocation there.

Also, it sounds like small memory leaks are expected when calling ExitThread, so maybe we shouldn't worry about this:

A thread in an executable that is linked to the static C run-time library (CRT) should use _beginthread and _endthread for thread management rather than CreateThread and ExitThread. Failure to do so results in small memory leaks when the thread calls ExitThread.

Yes, But different from CreateThread in static libraries in handling tlsdata issues
The memory allocated through Virtualalloc itself is not released.(56k bytes are leaked each time)
As a result, my unittest quickly omm

The following demo can be reproduced

#include <iostream> #include <thread> void test(int i) { std::cout << i << std::endl; } int main() { for (int i = 0; i < 10000; i++) { std::thread t(test, i); t.detach(); } char a; std::cin >> a; }

I'll try to hook ExitThread so that t can be released normally.

If it's just a dynamic library, can modify asan_thread_exit. Here intercept ExitThread for general purpose and leak g_native_tls_key(TlsAlloc)

You can review it when you have time

vitalybuka · 2025-02-20T18:09:49Z

compiler-rt/lib/asan/asan_win.cpp

@@ -166,6 +190,15 @@ INTERCEPTOR_WINAPI(HANDLE, CreateThread, LPSECURITY_ATTRIBUTES security,
                            thr_flags, tid);
 }

+INTERCEPTOR_WINAPI(void, ExitThread, DWORD dwExitCode) {
+  DWORD key = atomic_load(&g_native_tls_key, memory_order_relaxed);
+  AsanThread *t = (AsanThread *)TlsGetValue(key);


Isn't this the same as in SetCurrentThread ?

Different, SetCurrentThread calls AsanTSDSet to save the context

Of course there is another problem with the log

CHECK(t->context()); VReport(2, "SetCurrentThread: %p for thread %p\n", (void *)t->context(), (void *)GetThreadSelf()); // Make sure we do not reset the current AsanThread. CHECK_EQ(0, AsanTSDGet()); --- // In contrast to POSIX, on Windows GetCurrentThreadId() // returns a system-unique identifier. tid_t GetTid() { return GetCurrentThreadId(); } uptr GetThreadSelf() { return GetTid(); }

GetCurrentThreadId is returned a handle on Windows

So why we can't just GetCurrentThread()->Destroy()?

So why we can't just GetCurrentThread()->Destroy()?

Thanks, review it again when you have time

vitalybuka · 2025-02-27T05:19:43Z

compiler-rt/lib/asan/asan_win.cpp

@@ -166,6 +165,14 @@ INTERCEPTOR_WINAPI(HANDLE, CreateThread, LPSECURITY_ATTRIBUTES security,
                            thr_flags, tid);
 }

+INTERCEPTOR_WINAPI(void, ExitThread, DWORD dwExitCode) {
+  AsanThread *t = (AsanThread *)__asan::GetCurrentThread();
+  if (t) {


this code don't use {} in cases like these

vitalybuka

LGTM, but It would be nice to get from @rnk who requested changes.

Intercept `ExitThread` and free the memory created by `VirtualAlloc'

rnk

Thanks, I think this approach should work.

Fix llvm#126541 Since ```t->Destroy``` cannot be called after ```start_routine```(When calling standard thread_start in crt) Intercept `ExitThread` and free the memory created by `VirtualAlloc'

GkvJwa requested review from zmodem, vitalybuka and alvinhochun February 11, 2025 14:45

llvmbot added compiler-rt compiler-rt:asan Address sanitizer compiler-rt:sanitizer labels Feb 11, 2025

GkvJwa requested a review from rnk February 13, 2025 15:40

rnk requested changes Feb 13, 2025

View reviewed changes

GkvJwa force-pushed the fix_asan_leak branch from 04e9279 to d1a37b8 Compare February 16, 2025 05:15

GkvJwa requested a review from rnk February 16, 2025 05:24

GkvJwa force-pushed the fix_asan_leak branch 2 times, most recently from 22bcc77 to 2fcd13e Compare February 16, 2025 15:54

vitalybuka reviewed Feb 20, 2025

View reviewed changes

GkvJwa requested a review from vitalybuka February 25, 2025 08:18

GkvJwa force-pushed the fix_asan_leak branch from 2fcd13e to d85fea7 Compare February 25, 2025 09:48

vitalybuka reviewed Feb 27, 2025

View reviewed changes

vitalybuka approved these changes Feb 27, 2025

View reviewed changes

GkvJwa force-pushed the fix_asan_leak branch from d85fea7 to 630c45e Compare February 27, 2025 07:05

[asan][win] Fix ExitThread leak

630c45e

Intercept `ExitThread` and free the memory created by `VirtualAlloc'

rnk approved these changes Feb 27, 2025

View reviewed changes

GkvJwa merged commit 1594fa8 into llvm:main Feb 28, 2025
10 checks passed

GkvJwa deleted the fix_asan_leak branch February 28, 2025 01:38

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[asan][win] Fix CreateThread leak #126738

[asan][win] Fix CreateThread leak #126738

Uh oh!

GkvJwa commented Feb 11, 2025

Uh oh!

llvmbot commented Feb 11, 2025

Uh oh!

rnk Feb 13, 2025

Uh oh!

GkvJwa Feb 14, 2025 •

edited

Loading

Uh oh!

GkvJwa Feb 16, 2025 •

edited

Loading

Uh oh!

vitalybuka Feb 20, 2025

Uh oh!

GkvJwa Feb 21, 2025

Uh oh!

GkvJwa Feb 21, 2025

Uh oh!

vitalybuka Feb 25, 2025

Uh oh!

GkvJwa Feb 25, 2025

Uh oh!

vitalybuka Feb 27, 2025

Uh oh!

vitalybuka left a comment

Uh oh!

rnk left a comment

Uh oh!

Uh oh!

Uh oh!

[asan][win] Fix CreateThread leak #126738

[asan][win] Fix CreateThread leak #126738

Uh oh!

Conversation

GkvJwa commented Feb 11, 2025

Uh oh!

llvmbot commented Feb 11, 2025

Uh oh!

rnk Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

GkvJwa Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GkvJwa Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitalybuka Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

GkvJwa Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

GkvJwa Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

vitalybuka Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

GkvJwa Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

vitalybuka Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

vitalybuka left a comment

Choose a reason for hiding this comment

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GkvJwa Feb 14, 2025 •

edited

Loading

GkvJwa Feb 16, 2025 •

edited

Loading