Abort when there's an attempt to reinitialize a runtime thread object

[eduardo-vp]

Closes#112131.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @VSadov
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Adds a new CoreCLR baseservices test that attempts to execute managed code from a thread-destruction callback after the runtime has already torn down per-thread state, expecting the runtime to fail fast rather than re-attaching a new managed Thread object.

Changes:

• Introduces a new test project (ThreadStateDestroyed) configured for process isolation and crash-based pass criteria (expected exit codes).
• Adds a native test library that invokes a managed callback both during normal thread execution and again from a thread-destruction callback (Windows FLS / Unix pthread key destructor).
• Adds managed test logic that tracks whether the runtime re-attaches a new Thread on the second callback (should be unreachable if fail-fast works).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp	Native helper that runs a managed callback on a thread and again during thread teardown (FLS/pthread destructor).
src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyed.csproj	New SDK-style test project with isolation/crash expectations and native CMake project reference.
src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyed.cs	Managed entry point and UnmanagedCallersOnly callback used to detect (unexpected) thread re-attachment.
src/tests/baseservices/threading/ThreadStateDestroyed/CMakeLists.txt	Builds the native shared library used by the test.

Suppressed comments (1)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyed.cs:57

• Main reads s_callbackCount and s_secondCallbackGotNewThread after the native thread completes, but there is no managed synchronization edge between the foreign thread’s writes and these reads. Use Volatile.Read for the shared fields (and keep using the local copies for messaging) so the result is reliable under weak memory models.

 // Only reachable when the runtime did not fail fast.
if (s_callbackCount != 2)
{
Console.WriteLine($"[managed] Expected exactly 2 callbacks but got {s_callbackCount}.");
return 102;
}
if (!s_secondCallbackGotNewThread)
{
Console.WriteLine("[managed] The second callback reused the existing Thread.");
return 103;
}

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:95

• Same calling-convention issue as the Windows implementation: the non-Windows definition should also include STDMETHODCALLTYPE so the signature is consistent and the Windows x86 build doesn't accidentally pick up a cdecl definition when the #ifdef conditions change or get refactored.

extern "C" DLL_EXPORT void RunCallbackOnThreadAndDuringItsDestruction(CallbackFn callback)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:73

• AbortIfFail is a macro with an unparenthesized parameter and no do { } while (0) wrapper. As written it’s easy to misuse in an if/else and can also evaluate expressions with surprising precedence. Wrap it and parenthesize the argument to make it safe.

#define AbortIfFail(st) if (st != 0) abort()

[eduardo-vp]

The results of the test are as expected: on linux it fails but EBR fires first and on macOS we can see the actual bug because it doesn't detect re-initialization. After the fix the test should pass on all legs.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:80

• On POSIX, pthread key destructors may be invoked repeatedly (up to PTHREAD_DESTRUCTOR_ITERATIONS) if the key’s value remains non-null after the destructor runs. Since this destructor doesn’t clear the key, a non-failfast runtime could end up invoking the managed callback multiple times during teardown, adding noise and making failures harder to interpret. Clearing the key value here makes the behavior deterministic.

static void KeyDestructor(void*)
{
RunCallbackDuringThreadDestruction();
}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:6

• On Windows x86, delegate* unmanaged<void> uses the platform default (Winapi/stdcall). This file declares CallbackFn with the compiler default calling convention (cdecl), so calling the managed callback can corrupt the stack on 32-bit Windows. Match the callback type to Winapi/stdcall (and keep it a no-op on Unix where STDMETHODCALLTYPE is empty).

typedef void (*CallbackFn)();

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:46

• The exported entrypoint is invoked via [DllImport] without an explicit calling convention, so it uses Winapi (stdcall on Windows x86). This export currently has the compiler default calling convention (cdecl), which can corrupt the stack on 32-bit Windows. Add STDMETHODCALLTYPE to match the P/Invoke default (no effect on Unix).

extern "C" DLL_EXPORT void RunCallbackOnThreadAndDuringItsDestruction(CallbackFn callback)

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyed.cs:77

• Environment.ProcessPath can be null; other tests in this repo guard it before using it as ProcessStartInfo.FileName. Guarding avoids an unrelated NullReference/ArgumentNull failure mode and makes the test failure clearer if it ever happens.

 ProcessStartInfo psi = new ProcessStartInfo(Environment.ProcessPath, arguments)

src/coreclr/vm/ceemain.h:51

• The comment says this helper "fails fast" unconditionally, but the implementation intentionally becomes a no-op in free builds once any IJW module has been loaded (it uses _ASSERTE, which is compiled out). Update the comment to reflect that this is conditionally enforced.

// Fails fast if the runtime thread state of the current thread has already been destroyed.
void CheckThreadStateNotDestroyed();

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/coreclr/vm/ceemain.cpp:1813

• On Unix, ThreadStateKeyDestructor calls pthread_setspecific to restore the key value. Per POSIX, setting a non-NULL value from a key destructor causes the destructor to be invoked again in subsequent destructor-iteration passes (up to PTHREAD_DESTRUCTOR_ITERATIONS) for every terminating thread. That means this adds extra destructor callbacks and pthread_setspecific calls on every managed-thread teardown. Consider using a marker that doesn’t require re-setting the key from its own destructor (e.g., a PLATFORM_THREAD_LOCAL flag set in TlsDestructionMonitor), or otherwise structuring this so it doesn’t force repeated destructor iterations.

static void ThreadStateKeyDestructor(void* state)
{
if (state == &g_threadStateDestroyedMarker)
{
SetThreadStateDestroyed();

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/coreclr/vm/ceemain.cpp:1894

• CheckThreadStateNotDestroyed is documented as “fails fast”, but in _DEBUG builds the current _ASSERTE_ALL_BUILDS(!"...") expands to _ASSERTE, which does not reliably terminate the process. In non-_DEBUG VM builds it routes through EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE) and drops the assertion text, so the new test’s message check may fail on release legs. Consider using EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE for the non-IJW case to guarantee fail-fast behavior and a stable diagnostic string across configurations.

 if (Module::HasAnyIJWBeenLoaded())

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyed.cs:32

• On Windows x86, [DllImport] defaults to Winapi (stdcall), but the native export is declared without an explicit calling convention (so it will be __cdecl by default). This can cause stack imbalance or even failure to resolve the entry point on 32-bit Windows. Specify CallingConvention.Cdecl (or update the native export to STDMETHODCALLTYPE and keep the default) so managed and native agree across architectures.

 [DllImport(NativeLib)]
private static extern void RunCallbackOnThreadAndDuringItsDestruction(delegate* unmanaged<void> callback);

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

[Copilot]

🟡 Changes recommended

The Unix pthread-key “destroyed state” marker restoration in the key destructor is not robust (can trigger repeated destructor-iteration passes and still depends on unspecified key-destructor ordering).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/tests/baseservices/threading/ThreadStateDestroyed/ThreadStateDestroyedNative.cpp:4

• This file uses printf/fflush/abort but doesn't include the standard headers that declare them. In C++ this can be a hard compile error depending on toolchain/flags. Add the appropriate includes explicitly rather than relying on transitive headers.

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[eduardo-vp]

According to glibc docs thread local data data is destroyed before thread-specific data so it shouldn't be used in thread specific data destructors (https://sourceware.org/glibc/manual/2.40/html_node/Thread_002dspecific-Data.html).

The marker is set to null right before executing ThreadStateKeyDestructor and becomes non-null before it finishes, the other destructors should be able to see it.

[jkotas]

https://sourceware.org/glibc/manual/2.40/html_node/Thread_002dspecific-Data.html

I think it just means that the C++ destructors run. I do not think it means that TLS memory is freed. The memory for TLS is freed very late after all user code and callbacks run.

We have PLATFORM_THREAD_LOCAL macro that defines raw thread static storage and that we try to use everywhere we can. C++ thread statics come with bunch of goo that just causes all sort of problems. I think if you have used PLATFORM_THREAD_LOCAL, it would avoid the problem that Copilot pointed out.

[eduardo-vp]

Actually using PLATFORM_THREAD_LOCAL was my first attempt 571ed02 and worked on all platforms except osx x64 and arm64. All the subsequent commits were attempts to fix it on osx but so far none did.

I guess we use PLATFORM_THREAD_LOCAL in general and use something different on osx only.

[jkotas]

Why is PLATFORM_THREAD_LOCAL not working on osx?

[jkotas]

Is there a problem with _ASSERTE_ALL_BUILDS on Unix? I would rather fix that.