-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix lookup for current Thread in async signal handler #122048
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,125 @@ | ||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||
|
|
||||||||
| #include "common.h" | ||||||||
|
|
||||||||
| #include "asyncsafethreadmap.h" | ||||||||
|
|
||||||||
| // Async safe lock free thread map for use in signal handlers | ||||||||
|
|
||||||||
| struct ThreadEntry | ||||||||
| { | ||||||||
| size_t osThread; | ||||||||
| void* pThread; | ||||||||
| }; | ||||||||
|
|
||||||||
| #define MAX_THREADS_IN_SEGMENT 256 | ||||||||
|
|
||||||||
| struct ThreadSegment | ||||||||
| { | ||||||||
| ThreadEntry entries[MAX_THREADS_IN_SEGMENT]; | ||||||||
| ThreadSegment* pNext; | ||||||||
| }; | ||||||||
|
|
||||||||
| static ThreadSegment *s_pAsyncSafeThreadMapHead = NULL; | ||||||||
|
|
||||||||
| bool InsertThreadIntoAsyncSafeMap(size_t osThread, void* pThread) | ||||||||
| { | ||||||||
| size_t startIndex = osThread % MAX_THREADS_IN_SEGMENT; | ||||||||
|
|
||||||||
| ThreadSegment* pSegment = s_pAsyncSafeThreadMapHead; | ||||||||
| ThreadSegment** ppSegment = &s_pAsyncSafeThreadMapHead; | ||||||||
| while (true) | ||||||||
| { | ||||||||
| if (pSegment == NULL) | ||||||||
| { | ||||||||
| // Need to add a new segment | ||||||||
| ThreadSegment* pNewSegment = new (nothrow) ThreadSegment(); | ||||||||
| if (pNewSegment == NULL) | ||||||||
| { | ||||||||
| // Memory allocation failed | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| memset(pNewSegment, 0, sizeof(ThreadSegment)); | ||||||||
| ThreadSegment* pExpected = NULL; | ||||||||
| if (!__atomic_compare_exchange_n( | ||||||||
| ppSegment, | ||||||||
| &pExpected, | ||||||||
| pNewSegment, | ||||||||
| false /* weak */, | ||||||||
| __ATOMIC_RELEASE /* success_memorder */, | ||||||||
| __ATOMIC_RELAXED /* failure_memorder */)) | ||||||||
| { | ||||||||
| // Another thread added the segment first | ||||||||
| delete pNewSegment; | ||||||||
| pNewSegment = *ppSegment; | ||||||||
| } | ||||||||
|
|
||||||||
| pSegment = pNewSegment; | ||||||||
| } | ||||||||
| for (size_t i = 0; i < MAX_THREADS_IN_SEGMENT; i++) | ||||||||
| { | ||||||||
| size_t index = (startIndex + i) % MAX_THREADS_IN_SEGMENT; | ||||||||
|
|
||||||||
| size_t expected = 0; | ||||||||
| if (__atomic_compare_exchange_n( | ||||||||
| &pSegment->entries[index].osThread, | ||||||||
| &expected, osThread, | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| false /* weak */, | ||||||||
| __ATOMIC_RELEASE /* success_memorder */, | ||||||||
| __ATOMIC_RELAXED /* failure_memorder */)) | ||||||||
| { | ||||||||
| // Successfully inserted | ||||||||
| // Use atomic store with release to ensure proper ordering | ||||||||
| __atomic_store_n(&pSegment->entries[index].pThread, pThread, __ATOMIC_RELEASE); | ||||||||
| return true; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| ppSegment = &pSegment->pNext; | ||||||||
| pSegment = __atomic_load_n(&pSegment->pNext, __ATOMIC_ACQUIRE); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| void RemoveThreadFromAsyncSafeMap(size_t osThread, void* pThread) | ||||||||
| { | ||||||||
| size_t startIndex = osThread % MAX_THREADS_IN_SEGMENT; | ||||||||
|
|
||||||||
| ThreadSegment* pSegment = s_pAsyncSafeThreadMapHead; | ||||||||
| while (pSegment) | ||||||||
| { | ||||||||
| for (size_t i = 0; i < MAX_THREADS_IN_SEGMENT; i++) | ||||||||
| { | ||||||||
| size_t index = (startIndex + i) % MAX_THREADS_IN_SEGMENT; | ||||||||
| if (pSegment->entries[index].pThread == pThread) | ||||||||
| { | ||||||||
| // Found the entry, remove it | ||||||||
| pSegment->entries[index].pThread = NULL; | ||||||||
| __atomic_exchange_n(&pSegment->entries[index].osThread, (size_t)0, __ATOMIC_RELEASE); | ||||||||
| return; | ||||||||
| } | ||||||||
| } | ||||||||
| pSegment = __atomic_load_n(&pSegment->pNext, __ATOMIC_ACQUIRE); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| void *FindThreadInAsyncSafeMap(size_t osThread) | ||||||||
| { | ||||||||
| size_t startIndex = osThread % MAX_THREADS_IN_SEGMENT; | ||||||||
| ThreadSegment* pSegment = s_pAsyncSafeThreadMapHead; | ||||||||
| while (pSegment) | ||||||||
| { | ||||||||
| for (size_t i = 0; i < MAX_THREADS_IN_SEGMENT; i++) | ||||||||
| { | ||||||||
| size_t index = (startIndex + i) % MAX_THREADS_IN_SEGMENT; | ||||||||
| // Use acquire to synchronize with release in insert_thread_to_async_safe_map | ||||||||
|
||||||||
| // Use acquire to synchronize with release in insert_thread_to_async_safe_map | |
| // Use acquire to synchronize with release in minipal_insert_thread_into_async_safe_map |
Copilot
AI
Nov 28, 2025
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.
The pThread field is read non-atomically at line 122, which can race with the non-atomic write at line 78 in minipal_insert_thread_into_async_safe_map. Even though the osThread field is read with acquire semantics, this doesn't protect the subsequent pThread read because it's a separate memory location.
The pThread read should use an atomic load with acquire semantics to properly synchronize with the insert operation:
void* result = __atomic_load_n(&pSegment->entries[index].pThread, __ATOMIC_ACQUIRE);
return result;This ensures that if we observe a matching osThread value, we'll also observe the correctly written pThread value.
| return pSegment->entries[index].pThread; | |
| return __atomic_load_n(&pSegment->entries[index].pThread, __ATOMIC_ACQUIRE); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| #ifndef __ASYNCSAFETHREADMAP_H__ | ||
| #define __ASYNCSAFETHREADMAP_H__ | ||
|
|
||
| #if defined(TARGET_UNIX) && !defined(TARGET_WASM) | ||
|
|
||
| // Insert a thread into the async-safe map. | ||
| // * osThread - The OS thread ID to insert. | ||
| // * pThread - A pointer to the thread object to associate with the OS thread ID. | ||
| // * return true if the insertion was successful, false otherwise (OOM). | ||
| bool InsertThreadIntoAsyncSafeMap(size_t osThread, void* pThread); | ||
|
|
||
| // Remove a thread from the async-safe map. | ||
| // * osThread - The OS thread ID to remove. | ||
| // * pThread - A pointer to the thread object associated with the OS thread ID. | ||
| void RemoveThreadFromAsyncSafeMap(size_t osThread, void* pThread); | ||
|
|
||
| // Find a thread in the async-safe map. | ||
| // * osThread = The OS thread ID to search for. | ||
| // * return - A pointer to the thread object associated with the OS thread ID, or NULL if not found. | ||
| void* FindThreadInAsyncSafeMap(size_t osThread); | ||
|
|
||
| #endif // TARGET_UNIX && !TARGET_WASM | ||
|
|
||
| #endif // __ASYNCSAFETHREADMAP_H__ |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5731,16 +5731,18 @@ void ThreadSuspend::SuspendEE(SUSPEND_REASON reason) | |||||||||||
| // It is unsafe to use blocking APIs or allocate in this method. | ||||||||||||
| BOOL CheckActivationSafePoint(SIZE_T ip) | ||||||||||||
| { | ||||||||||||
| Thread *pThread = GetThreadNULLOk(); | ||||||||||||
| Thread *pThread = GetThreadAsyncSafe(); | ||||||||||||
| _ASSERTE(pThread != NULL); | ||||||||||||
|
|
||||||||||||
|
Comment on lines
+5735
to
5736
|
||||||||||||
| _ASSERTE(pThread != NULL); | |
| if (pThread == NULL) | |
| { | |
| return FALSE; | |
| } |
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.
This looks like valid feedback. We can be seeing a signal sent by some other component (e.g. second NAOT runtime). We should forward the signal to the next component in that case.
Could you please do some ad-hoc stress testing to make sure that it works well? We should not be crashing, and we should not be eating signals that other components may expect.
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.
This assert is a forgotten leftover from my local testing, it should not be there.
Could you please do some ad-hoc stress testing to make sure that it works well? We should not be crashing, and we should not be eating signals that other components may expect.
Yes, I'll do that.
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.
Instead of reaching back up and accessing a potentially changing value, this API writes the non-equal failure value back into
pExpected. I think the more correct operation would bepNewSegment = *pExpected;