[release/5.0] Fix covariant returns with generic return types - #45873
Merged
Conversation
When a covariant return type was an uninstantiated generic type, the ClassLoader::IsCompatibleWith was not working properly. In debug builds, it was asserting because there was no MethodTable for that type and in release builds, it resulted in ExecutionEngineException or an internal CLR error. This change fixes it by using TypeHandle::CanCastTo instead of MethodTable::CanCastTo and adds a regression test for two cases where the problem was observed (Assembly.GetTypes() and creating an instance of a type with a covariant return with a problematic kind of type).
There were two issues. First, the ClassLoader::ValidateMethodsWithCovariantReturnTypes was called before typeHnd.DoFullyLoad and that resulted in an assert down the call chain of TypeDesc::CanCastTo due to a wrong load level. Second, the SigTypeContext generation for the current MD in the ClassLoader::ValidateMethodsWithCovariantReturnTypes requires the same change for class instantiation as the one that we had for the parent MD.
The call to ClassLoader::ValidateMethodsWithCovariantReturnTypes is now in MethodTable::DoFullyLoad. I have also added a test case that verifies a case that David Wrighton has suggested offline, where there are 3 types... A, B and C. C derives from B which derives from A. B has a bad override which should produce an error. Then, cause C to be fully loaded without otherwise triggering a load of B.
jkotas
approved these changes
Dec 10, 2020
This was referenced Dec 10, 2020
jeffschwMSFT
approved these changes
Dec 10, 2020
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
Member
There was a problem hiding this comment.
Approved. We will consider this for 5.0.2
Member
|
Approved during tactics. |
ghost
locked as resolved and limited conversation to collaborators
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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 #45275 to release/5.0
/cc @janvorli
Customer Impact
Valid application that uses generic types in covariant returns crashes at runtime with System.TypeLoadException when attempting to use the underlying type or enumerating types in the assembly using Assembly.GetTypes().
Testing
CoreCLR pri1 tests, including two new tests added that specifically target the problematic cases (copies of the repros provided by our users that have found the issue)
Risk
Low
Regression
No