Fix locking around m_LoaderAllocatorReferences iteration #68336
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The iteration that marks loader allocators in LoaderAllocator::Mark() was not being called under an appropriate lock, which lead to a rare and hard to reproduce race condition and a crash in the LoaderAllocator::Mark(). The issue happened when the m_LoaderAllocatorReferences.Begin() returned an iterator at the time the m_LoaderAllocatorReferences were empty and another thread has written something into those references before the m_LoaderAllocatorReferences.End() was called. The iter was then evaluated as not equal to the End and the iter was getting dereferenced. Since the iterator had m_table set to NULL, the dereferencing caused crash.
jkotas
reviewed
Apr 21, 2022
| @@ -384,6 +384,7 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain | |||
| AppDomain::AssemblyIterator i; | |||
| // Iterate through every loader allocator, marking as we go | |||
| { | |||
| CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock()); | |||
| CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock()); | |||
There was a problem hiding this comment.
The second iteration below is going to acquire same locks now. We can acquire the locks just once for the whole duration of both iterations.
There was a problem hiding this comment.
Makes sense, I'll update the PR that way.
jkotas
approved these changes
Apr 21, 2022
|
(setting the milestone to |
janvorli
added a commit
to janvorli/runtime
that referenced
this pull request
Apr 28, 2022
This change ports two unloadability fixes for issues found by a customer. * dotnet#68550 that adds reference between native `LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of them is used to resolve assembly using the other one. This reference ensures that the one that provides the resolved `Assembly` isn't collected before the one that uses it. * dotnet#68336 that adds a missing lock around `m_LoaderAllocatorReferences` iteration during assembly load context destruction.
carlossanlop
pushed a commit
that referenced
this pull request
May 5, 2022
* [Release/6.0] Port unloadability fixes This change ports two unloadability fixes for issues found by a customer. * #68550 that adds reference between native `LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of them is used to resolve assembly using the other one. This reference ensures that the one that provides the resolved `Assembly` isn't collected before the one that uses it. * #68336 that adds a missing lock around `m_LoaderAllocatorReferences` iteration during assembly load context destruction. * Remove breaking change part The change in main has added an exception in case a collectible `Assembly` is resolved into a non-collectible `AssemblyLoadContext`. Although that is incorrect, there are cases when it would work. So the added exception is a breaking change that I think we likely don't want to get into 6.0
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The iteration that marks loader allocators in LoaderAllocator::Mark()
was not being called under an appropriate lock, which lead to a rare and
hard to reproduce race condition and a crash in the
LoaderAllocator::Mark(). The issue happened when the
m_LoaderAllocatorReferences.Begin() returned an iterator at the time the
m_LoaderAllocatorReferences were empty and another thread has written
something into those references before the
m_LoaderAllocatorReferences.End() was called. The iter was then
evaluated as not equal to the End and the iter was getting dereferenced.
Since the iterator had m_table set to NULL, the dereferencing caused
crash.
The issue shows up as something like this: