[release/5.0-rc2] AccessViolation when using ValueTypes as a Service #42213
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}
When using ValueTypes as services in DependencyInjection, we are generating incorrect IL in a few cases: - When the ValueType is a top level service - When the ValueType is in an IEnumerable of services We were double unboxing the ValueType, which was causing AccessViolations. This was a regression caused by a previous change when ValueTypes were used in constructor parameters of services. To fix this, I lifted the unboxing into the VisitConstructor and VisitIEnumerable methods instead of forcing VisitConstant to always do the unboxing. VisitConstant doesn't have enough context to know whether it should do the unboxing or not. Fix #42037
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
eerhardt
added
area-Extensions-DependencyInjection
Servicing-consider
|
Tagging subscribers to this area: @eerhardt, @maryamariyan |
ericstj
approved these changes
Sep 14, 2020
|
test failure is #42225 |
leecow
added
Servicing-approved
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.
Backport of #42152 to release/5.0-rc2
/cc @eerhardt
Customer Impact
When a consumer of DependencyInjection is using a
structtype as a service, we are generating incorrect IL. We can get scenarios of double unboxing a struct, or using a value type as anobjectreference. Both of which can cause memory corruption and Access Violations.This was a regression introduced early in .NET 5 which was fixing a different IL generation problem. Both the original issue and the regression are fixed by this change.
Testing
I added new regression tests that cover the affected scenario. Also ran the repro project reported by the customer, and the issue is fixed.
@ericstj and I statically analyzed the cases that could run into this issue and found another place - when using a collection of value types. Added a test case for this as well.
Risk
Since anything to do with IL generation is risky, I would say this is "medium" risk. The applied fix only affects value types, so typical usage of DI shouldn't be affected. All current and new tests pass with this change.