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.

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

Disallow resetting initial thread apartment state #72332

Merged
merged 1 commit into from
Jul 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,13 @@ private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
{
if (HasStarted())
throw new ThreadStateException();

// Compat: Disallow resetting the initial apartment state
if (_initialApartmentState == state)
return true;
if (_initialApartmentState != ApartmentState.Unknown)
return false;

_initialApartmentState = state;
return true;
}
Expand Down
54 changes: 5 additions & 49 deletions src/coreclr/vm/comsynchronizable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -686,25 +686,11 @@ FCIMPL2(INT32, ThreadNative::SetApartmentState, ThreadBaseObject* pThisUNSAFE, I
if (pThisUNSAFE==NULL)
FCThrowRes(kNullReferenceException, W("NullReference_This"));

INT32 retVal = ApartmentUnknown;
BOOL ok = TRUE;
THREADBASEREF pThis = (THREADBASEREF) pThisUNSAFE;

HELPER_METHOD_FRAME_BEGIN_RET_1(pThis);

// Translate state input. ApartmentUnknown is not an acceptable input state.
// Throw an exception here rather than pass it through to the internal
// routine, which asserts.
Thread::ApartmentState state = Thread::AS_Unknown;
if (iState == ApartmentSTA)
state = Thread::AS_InSTA;
else if (iState == ApartmentMTA)
state = Thread::AS_InMTA;
else if (iState == ApartmentUnknown)
state = Thread::AS_Unknown;
else
COMPlusThrow(kArgumentOutOfRangeException, W("ArgumentOutOfRange_Enum"));

Thread *thread = pThis->GetInternal();
if (!thread)
COMPlusThrow(kThreadStateException, IDS_EE_THREAD_CANNOT_GET);
Expand All @@ -722,7 +708,7 @@ FCIMPL2(INT32, ThreadNative::SetApartmentState, ThreadBaseObject* pThisUNSAFE, I
{
EX_TRY
{
state = thread->SetApartment(state);
iState = thread->SetApartment((Thread::ApartmentState)iState);
}
EX_CATCH
{
Expand All @@ -735,24 +721,13 @@ FCIMPL2(INT32, ThreadNative::SetApartmentState, ThreadBaseObject* pThisUNSAFE, I
pThis->LeaveObjMonitor();
}


// Now it's safe to throw exceptions again.
if (!ok)
COMPlusThrow(kThreadStateException);

// Translate state back into external form
if (state == Thread::AS_InSTA)
retVal = ApartmentSTA;
else if (state == Thread::AS_InMTA)
retVal = ApartmentMTA;
else if (state == Thread::AS_Unknown)
retVal = ApartmentUnknown;
else
_ASSERTE(!"Invalid state returned from SetApartment");

HELPER_METHOD_FRAME_END();

return retVal;
return iState;
}
FCIMPLEND

Expand Down Expand Up @@ -780,41 +755,22 @@ FCIMPL1(INT32, ThreadNative::GetApartmentState, ThreadBaseObject* pThisUNSAFE)
COMPlusThrow(kThreadStateException, W("ThreadState_Dead_State"));
}

Thread::ApartmentState state = thread->GetApartment();
retVal = thread->GetApartment();

#ifdef FEATURE_COMINTEROP
if (state == Thread::AS_Unknown)
if (retVal == Thread::AS_Unknown)
{
// If the CLR hasn't started COM yet, start it up and attempt the call again.
// We do this in order to minimize the number of situations under which we return
// ApartmentState.Unknown to our callers.
if (!g_fComStarted)
{
EnsureComStarted();
state = thread->GetApartment();
retVal = thread->GetApartment();
}
}
#endif // FEATURE_COMINTEROP

// Translate state into external form
retVal = ApartmentUnknown;
if (state == Thread::AS_InSTA)
{
retVal = ApartmentSTA;
}
else if (state == Thread::AS_InMTA)
{
retVal = ApartmentMTA;
}
else if (state == Thread::AS_Unknown)
{
retVal = ApartmentUnknown;
}
else
{
_ASSERTE(!"Invalid state returned from GetApartment");
}

HELPER_METHOD_FRAME_END();

return retVal;
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/vm/comsynchronizable.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,6 @@ friend class ThreadBaseObject;
ThreadAbortRequested = 128,
};

enum
Copy link
Member Author

Choose a reason for hiding this comment

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

This enum was redundant. I have deleted it and the remapping code for enum values.

{
ApartmentSTA = 0,
ApartmentMTA = 1,
ApartmentUnknown = 2
};

static FCDECL1(INT32, GetPriority, ThreadBaseObject* pThisUNSAFE);
static FCDECL2(void, SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority);
static FCDECL1(void, Interrupt, ThreadBaseObject* pThisUNSAFE);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/threads.h
Original file line number Diff line number Diff line change
Expand Up @@ -2654,7 +2654,9 @@ class Thread
// undecided. The last state may indicate that the apartment has not been set at
// all (nobody has called CoInitializeEx) or that the EE does not know the
// current state (EE has not called CoInitializeEx).
// Keep in sync with System.Threading.ApartmentState
enum ApartmentState { AS_InSTA, AS_InMTA, AS_Unknown };

ApartmentState GetApartment();
ApartmentState GetApartmentRare(Thread::ApartmentState as);
ApartmentState GetExplicitApartment();
Expand Down
2 changes: 0 additions & 2 deletions src/libraries/System.Threading.Thread/tests/ThreadTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ public static void GetSetApartmentStateTest_ChangeAfterThreadStarted_Windows(

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34543", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/72232", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
[MemberData(nameof(ApartmentStateTest_MemberData))]
[PlatformSpecific(TestPlatforms.Windows)] // Expected behavior differs on Unix and Windows
public static void ApartmentStateTest_ChangeBeforeThreadStarted_Windows(
Expand All @@ -278,7 +277,6 @@ public static void ApartmentStateTest_ChangeBeforeThreadStarted_Windows(

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsWindowsNanoServer))]
[MemberData(nameof(ApartmentStateTest_MemberData))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/72232", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
public static void ApartmentStateTest_ChangeBeforeThreadStarted_Windows_Nano_Server(
Func<Thread, ApartmentState> getApartmentState,
Func<Thread, ApartmentState, int> setApartmentState,
Expand Down