Skip to content
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.

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

Fix for issue 70385 (stack overflow in the CoreCLR runtime during SVM validation) #71135

Merged
merged 5 commits into from
Jun 22, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Jun 22, 2022

The issue is caused by the fact that in the presence of recursive generics
verification that all SVMs are implemented ends up loading base types
with the full (CLASS_LOADED) level; if the base type is generic and contains
the type being verified as a type parameter, it causes infinite recursion.
I have adjusted two places around the SVM resolution to only require
CLASS_LOAD_EXACTPARENTS to prevent this. I have created a regression
test for the issue and I verified locally that it crashes with stack overflow
without my fix and passes with it.

Thanks

Tomas

Fixes: #70385


interface IDerived<T> : IBase<T>
{
public static void IBase<T>.Method()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the public in this line is redundant and causing compilation to fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your feedback, I have removed the public modifier from both interfaces in the 2nd commit. Please note that the C# code is actually not used for building the test - that's why it has the 'template' extension - because we don't yet have a Roslyn capable of compiling this code, the actual test source code is written in IL.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

This change adds the possibility that we may attempt to utilize a MethodDesc that is not fully loaded within the Jit interface and within the generic dictionary resolution logic in genericdict.cpp. I would like to see code in MethodTable::TryResolveConstraintMethodApprox which explicitly forces the level to CLASS_LOADED.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 22, 2022
@davidwrighton
Copy link
Member

Oh, also you need to make sure that the appropriate level is loaded in RuntimeTypeHandle_GetInterfaceMethodImplementation. I recommend following the default parameter pattern.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 22, 2022
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Looks good.

@trylek
Copy link
Member Author

trylek commented Jun 22, 2022

The library failure is known, Carlos is working on the fix, #71038, merging in.

@trylek trylek merged commit df3bd55 into dotnet:main Jun 22, 2022
@trylek trylek deleted the SVMStackOverflow branch June 22, 2022 23:53
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow in crossgen2
3 participants