[release/8.0-staging] Fix AsyncVoidMethodBuilder race condition around SynchronizationContext #99640
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This fixes a long-standing issue we've seen sporadically over the years but for which we just got a solid repro; the symptom is a sporadic unhandled null reference exception that crashes an app when using an async void method builder and a non-default SynchronizationContext. The issue is that, because of how state management is handled in the builder, the builder itself can be cleared while its SetResult method is running, and that means two reads of the _synchronizationContext field can end up returning a non-null value followed by a null value. The fix is to just cache the field into a local before completing the builder, and then only use the local state after.
|
Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices |
stephentoub
added
Servicing-consider
kouvel
approved these changes
Mar 13, 2024
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.
Backport of #99461 to release/8.0-staging
/cc @stephentoub
Customer Impact
Sporadic unhandleable crashes when using async void methods and there's a custom SynchronizationContext in play. There's a race condition that can result in the state of an object being cleared while that object is still in use, resulting in two reads of the same field first returning a non-null value and then returning a null value; the result of the second read is then dereferenced, producing a NullReferenceException that crashes the process.
Regression
Introduced in .NET Core 3.0. We've heard sporadic reports about the problem since but have never had a good repro to narrow it down.
Testing
A solid customer provided repro was provided that quickly crashed before the fix. After the fix, the issue no longer repros. It's also easy to see from code inspection that the problem can no longer occur.
Risk
Low. It's effectively just caching a read of a field into a local rather than reading that field twice (it also moves the read a bit earlier, but the data is observably readonly from construction, so moving it earlier doesn't matter.