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

Devirtualization in CrossGen2 for internal virtual overrides mismatches runtime behaviour #60969

Open
AnakinRaW opened this issue Oct 28, 2021 · 4 comments

Comments

@AnakinRaW
Copy link
Contributor

AnakinRaW commented Oct 28, 2021

Description

While playing around with the virtual method resolution algorithm in the crossgen project I noticed that the resolution algorithm does not handle a rare (synthetical) scenario where overriding methods are internal and made available for certain subclasses via the attribute InternalsVisibleTo.

While the runtime handles this accordingly with a MethodAccessException the method resolution for crossgen apparently does consider accessibility (at least it seems like it).

Reproduction Steps

I created the following assemblies:

.assembly A
{
  .custom instance void [System.Runtime]System.Runtime.CompilerServices.InternalsVisibleToAttribute::.ctor(string) = ( 01 00 01 42 00 00 )                               // ...B..
}
.mscorlib
.class public auto ansi beforefieldinit A.ClassA
{
  .method assembly hidebysig newslot strict virtual 
          instance void  M() cil managed { }
}
.assembly extern A {}
.assembly B {}
.mscorlib
.class public auto ansi beforefieldinit B.ClassB
       extends [A]A.ClassA { }
.assembly extern A { }
.assembly C { }
.mscorlib
.class public auto ansi beforefieldinit C.ClassC
       extends [A]A.ClassA { }

I conducted the following test case:

var aAssm = _context.GetModuleForSimpleName("A");
var bAssm = _context.GetModuleForSimpleName("B");
var cAssm = _context.GetModuleForSimpleName("C");
MetadataType a = aAssm.GetType("A", "ClassA");
MetadataType b = bAssm.GetType("B", "ClassB");
MetadataType c = cAssm.GetType("C", "ClassC");
MethodDesc aMethod = a.GetMethods().Where(m => m.Name == "M").Single();

{
    var result = new DevirtualizationManager().ResolveVirtualMethod(aMethod, a, out _); // Returns [A]AClass::M
    result = new DevirtualizationManager().ResolveVirtualMethod(aMethod, b, out _); // Returns [A]AClass::M
    result = new DevirtualizationManager().ResolveVirtualMethod(aMethod, c, out _); // Returns [A]AClass::M
}

Expected behavior

Running the same assemblies with with a given Main method in ClassC

newobj     instance void C.ClassC::.ctor()
callvirt   instance void [A]A.ClassA::M()

yields a System.MethodAccessException: Attempt by method 'C.ClassC.Main(System.String[])' to access method 'A.ClassA.M()' failed., as expected.

However when the provided test case above returns [A]A.ClassA::M(). The out parameter of the devirtualization has default value CORINFO_DEVIRTUALIZATION_UNKNOWN.

The Roslyn Compiler guys already covered this issue to explicitly deny compilation in such a scenario. so maybe this helps in case my own words are too confusing ;)
https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs#L2044

Actual behavior

The result value on the 3rd case is expected to be null because at runtime such an operation would fail in a MethodAccessException.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 28, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@AnakinRaW AnakinRaW changed the title Devirtualization for internal virtual overrides mismatches runtime behaviour Devirtualization in CrossGen2 for internal virtual overrides mismatches runtime behaviour Oct 28, 2021
@MichalStrehovsky
Copy link
Member

Crossgen2 doesn't do access checks in general. E.g. reading from a field that is not visible/accessible is also allowed:

// TODO: We need to implement access checks for fields and methods. See JitInterface.cpp in mrtjit
// and STS::AccessCheck::CanAccess.

It's a bit of a niche compat problem.

@AnakinRaW
Copy link
Contributor Author

ah, that's good to know.
Are there any plans this gets included to Crossgen2? If not, could someone maybe give me a small hint where in MetadataVirtualMethodAlgorithm.cs or DevirtualizationManager.cs this check should be added.

@AnakinRaW
Copy link
Contributor Author

This is probably related to #61124. Without proper accessibility checking Crossgen probably also runs into this issue. Not tested though.

@trylek trylek removed the untriaged New issue has not been triaged by the area owner label Jun 15, 2022
@trylek trylek added this to the Future milestone Jun 15, 2022
@trylek trylek mentioned this issue May 3, 2023
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants