Runtime: Why does DisablePrivateReflection attribute not work on .net core 2.1, but works perfectly on .net core 2.0?

[dalekseev-monokot]

Hello!

Why does DisablePrivateReflection attribute not work on .net core 2.1, but works perfectly on .net core 2.0?

I have a class with private fields. The class is in assembly marked with a DisablePrivateReflection attribute.

The following test will be passed successfully on .net core 2.0

var field = typeof(MonLicenseManager).GetField("_licenses", BindingFlags.NonPublic | 
BindingFlags.Static);

Assert.IsNotNull(field);

try
{
    var value = field.GetValue(null);
    Assert.Fail("FieldAccessException must be thrown");
}
catch (FieldAccessException)
{

}

but on .net core 2.1, FieldAccessException will never be thrown

Thanks

[karelz]

That seems like the attribute actually does not work on 2.0, but works correctly ans as expected in 2.1.
That seems like by design technical breaking change to me.

@steveharter cc @joshfree

[steveharter]

I verified that it did work in 2.0.

It appears that there used to be a call to PerformVisibilityCheckOnField which was removed with https://github.com/dotnet/coreclr/pull/13241. That PR appears to address part of CAS removal issue https://github.com/dotnet/coreclr/issues/9321.

In 2.0, the call stack would look like this:

Unhandled Exception: System.FieldAccessException: Attempt by method 'test.Program.Main(System.String[])' to access field 'test.Foo.i' failed.
   at System.Reflection.RtFieldInfo.PerformVisibilityCheckOnField(IntPtr field, Object target, RuntimeType declaringType, FieldAttributes attr, UInt32 invocationFlags)
   at System.Reflection.RtFieldInfo.InternalGetValue(Object obj, StackCrawlMark& stackMark)
   at System.Reflection.RtFieldInfo.GetValue(Object obj)

@jkotas @danmosemsft is this expected per the removal of the CAS issue?

[jkotas]

This was not expected. We should keep respecting the DisablePrivateReflection attribute.

[ajcvickers]

@jkotas Two questions:

• Is this actually useful in any scenarios?
• Will attempts to create a compiled expression that accesses the field also fail?

[jkotas]

Is this actually useful in any scenarios?

This was meant to be used by library developers who do not wish to allow external code to hack the library internals. The alternative is obfuscation that comes with its own set of issues.

Will attempts to create a compiled expression that accesses the field also fail?

Was this case allowed in the .NET Framework implementation of this attribute?

I think we would try to match the .NET Framework behavior by default. However, I am not sure whether we would be able to match it exactly since this check was attached to code-access-security (CAS) that does exist in .NET Core.

[ajcvickers]

@jkotas I don't know if it works on .NET Framework or not--I never heard of it before this thread. As far as I'm aware, with .NET Framework, the accepted wisdom was that there is no foolproof way to prevent such access and as such it's usually futile to try. For example, this stack overflow thread. This thread has answers from both Marc Gravell and Eric Lippert, both of who are very reliable sources.

On EF Core, we've been working on the assumption that without CAS there is no way to try to prevent access to private members (which EF does _a lot_) so we have not been testing any negative cases where it is supposed to be blocked. If this attribute is a thing that's supposed to do that, then we may need to consider the impact.

I'm not really sure how this is related to obfuscation, since all that does it make it harder to understand what the private members are, but doesn't in any way prevent access to them.

/cc @divega

[jkotas]

Right, this attribute was a speed bump - in the same way as obfuscation is a speed bump.

[ajcvickers]

@jkotas Okay, thanks for the info.

[terrajobst]

@jkotas

Assuming we honor the attribute, how much of a performance penalty do we pay for success cases (i.e. when the attribute isn't applied)? In other words, is this an item where the majority has to pay for a feature only a few people will ever use?

[jkotas]

If we bring it back, it should have zero impact on the regular case (ie 100% pay-for-play).

It is why I have said that we may not be able to match the .NET Framework behavior exactly. We would not want to reintroduce the complex and expensive CAS checks from .NET Framework to match the behavior exactly.

[abatishchev]

Long story short: please don't bring it back. Imo, it should've never been existed in the first place.

[danmosemsft]

@jkotas @ajcvickers I'm moving this to Future as it does not seem essential for 3.0. Feel free to move back if I'm wrong.

[ajcvickers]

@danmosemsft Not in favor of it at all, so happy for you to punt it. :-)

[GrabYourPitchforks]

Looks like we've now gone two major releases (2.1, 3.x) without this functionality. Is there any plan to resurrect it for 5.0?

[MichalStrehovsky]

This attribute is interesting for two reasons:

• Size optimizations: e.g. IL Linker can safely strip names and attributes on things without having to worry that it might change semantics compared to the state before linking (modulo things like stack traces). We could even give non-generic types/members compilercontrolled visibility (i.e. members are referred to by tokens, not by names where we don't even have to bother generating short unique names for them). Cc @marek-safar, @vitek-karas
• Codegen optimization: none of the top-down codegen optimizations (the ones where in optimizing callee we assume we know values of parameters at all callsites) are safe to do in .NET because reflection allows calling anything with any values. When reflection is out of picture, these optimizations are safe. I know WASM has to do these illegal optimization to get size down (knowledge about parameters that are never passed as null). These optimizations would be safe when (at least the private) reflection can be disallowed and this is enforced by all runtimes. Cc @vargaz @AndyAyersMS @erozenfeld

[marek-safar]

I think it'd be useful to fix DisablePrivateReflectionAttribute to work the way that it provides better dev experience when something unexpectedly calls member via reflection.

[steveharter]

Moving to future due to priority + scheduling reasons.

[GrabYourPitchforks]

If punted, we will have gone __three__ major releases (2.1, 3.1, 5.0) without this functionality. I don't really see many people clamoring to bring this back. If we're unlikely ever to bring it back I'd rather close the issue (and obsolete the type) rather than continue punting.

[jkotas]

I believe that the linker annotations that we investing into are a better approach for taming reflection use. I agree that it is not worth spending time on DisablePrivateReflection.

[steveharter]

Agree. Closing.

[MichalStrehovsky]

Agree. Closing.

Do we have an issue tracking updating documentation?

[GrabYourPitchforks]

Opened https://github.com/dotnet/dotnet-api-docs/issues/4646 to track.