Skip to content

Fix error handling for COM initialization failures during thread startup #72353

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

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Fix error handling for COM initialization failures during thread startup #72353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 20 additions & 18 deletions ...coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ public ApartmentState GetApartmentState()

private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
{
ApartmentState retState;

if (this != CurrentThread)
{
using (LockHolder.Hold(_lock))
Expand All @@ -255,32 +257,32 @@ private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
throw new ThreadStateException();

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

_initialApartmentState = state;
return true;
retState = _initialApartmentState;
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 != CurrentThread) path needs to have the same error handling and throw InvalidOperationException as the (this == CurrentThread) path.

}
}

if ((t_comState & ComState.Locked) == 0)
else
{
if (state != ApartmentState.Unknown)
{
InitializeCom(state);
}
else

if ((t_comState & ComState.Locked) == 0)
{
UninitializeCom();
if (state != ApartmentState.Unknown)
{
InitializeCom(state);
}
else
{
UninitializeCom();
}
}
}

// Clear the cache and check whether new state matches the desired state
t_apartmentType = ApartmentType.Unknown;
// Clear the cache and check whether new state matches the desired state
t_apartmentType = ApartmentType.Unknown;

ApartmentState retState = GetApartmentState();
retState = GetApartmentState();
}

if (retState != state)
{
Expand Down
13 changes: 10 additions & 3 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public sealed partial class Thread
private ManagedThreadId _managedThreadId;
private string? _name;
private StartHelper? _startHelper;
private Exception? _startException;

// Protects starting the thread and setting its priority
private Lock _lock = new Lock();
Expand Down Expand Up @@ -367,9 +368,13 @@ private void StartCore()

if (GetThreadStateBit(ThreadState.Unstarted))
{
// Lack of memory is the only expected reason for thread creation failure
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 was incorrect assumption. COM initialization can fail with PlatformNotSupportedException on Nano server.

throw new ThreadStartException(new OutOfMemoryException());
Exception? startException = _startException;
startException = null;
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be _startException = null;


throw new ThreadStartException(startException ?? new OutOfMemoryException());
}

Debug.Assert(_startException == null);
}
}

Expand All @@ -384,8 +389,10 @@ private static void StartThread(IntPtr parameter)
System.Threading.ManagedThreadId.SetForCurrentThread(thread._managedThreadId);
thread.InitializeComOnNewThread();
}
catch (OutOfMemoryException)
catch (Exception e)
{
thread._startException = e;

#if TARGET_UNIX
// This should go away once OnThreadExit stops using t_currentThread to signal
// shutdown of the thread on Unix.
Expand Down