Warn when accessing DAM annotated method via reflection

[vitek-karas]

Any access to DAM annotated method must be validated by the trimmer. Normal calls and such are handled already, but direct reflection access or other indirect accesses can lead to trimmer allowing potential invocation of DAM annotated method without validating the annotations. This change will warn whenever such potential "leak" happens.

This applies to method parameter, method return value and field annotations.
Uses the same infra as RUC already just adds additional checks.

Tests for the new cases.

Fixes #1764

[vitek-karas]

I ran this on dotnet/runtime and it found a couple of places where it warns. Fixing them now as most are easy.
The result of this was the tweak to not warn on non-virtual return annotations - since that occurs in runtime and is just tedious (as it's clearly not a problem). I'll like a draft PR to runtime with the fixes here once ready.

[vitek-karas]

The changes to runtime are being done here: https://github.com/dotnet/runtime/compare/main...vitek-karas:FixReflectionOnDAMImpact?expand=1

Everything except EventSource is already resolved.

[sbomer]

I know we use this kind of pattern in a few places already (I added some of them myself 😦) but I think we should avoid it. Special-casing behavior for some "reason kinds" isn't very maintainable - it requires examining all callers of the method to see which cases should be treated specially. I would prefer to add a separate parameter to MarkField that indicates what the optional behavior is, and pass it in from some callers only.

[vitek-karas]

In general I agree, but it would require non-trivial effort. Not for fields, but for methods. The entire RUC and DAM detection in MarkMethod currently relies on this. If we were to fix it, I think we should do it in both places.

I did go through all the callers for both methods and fields, it's tedious I agree, but fixing it would require lot more work. I didn't want to block this change on it. Mainly because I want to avoid passing many different parameters to Mark methods. And MarkMethod would need at least 2 more (RUC and DAM handling). So in my mind a cleaner solution would be to do this in the planned refactoring of scope stacks and DependencyInfo into one parameter. With that we could relatively easily add more fields to the structure we will pass around and store additional information that way.

[vitek-karas]

This is ready for review.

That said we may want to hold off on merging this until we have #2136 resolved since this causes new warnings in EventSource which will require different supressions once that change is implemented.

[vitek-karas]

Ping. Could somebody please review this?

[MichalStrehovsky]

Looks good otherwise!

[MichalStrehovsky]

Feels like this one is forcing things into following a pattern but hurting readability in the process.

[vitek-karas]

I was on the fence about these - so if others like the more verbose shape... I changed the once which have conditions in them to normal if pattern.

[MichalStrehovsky]

Do we need a call to ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode?

[vitek-karas]

Great catch - thanks a lot!

[MichalStrehovsky]

We should also disallow this if the return value is a ref because one can just create a delegate from that and modify whatever the byref is pointing to without anyone checking.

[vitek-karas]

Can you give me an example? You mean something like:

[DAM(PublicMethods)]
Type _typeField;

[return: DAM(PublicMethods)]
ref Type GetRefType() { return ref _typeField; }

// But now what?

[MichalStrehovsky]

Along the lines of:

delegate ref Type Break();

var b = (Break)typeof(Foo).GetMethod("GetRefType").CreateDelegate(typeof(Break));
ref Type x = ref b();

// Nobody told us to keep methods on Bar
x = typeof(Bar);

[vitek-karas]

But x has no annotations on it (even implicitly), so if I add one more line:

x.GetMethods(); // Warns, since x doesn't have any annotations

[MichalStrehovsky]

_typeField is annotated though so we can get methods on it without warnings.

[vitek-karas]

I see - the magical line is:

_typeField.GetMethods(); // This doesn't warn, but it now contains typeof(Bar) which nobody checked

[vitek-karas]

This feels like a bigger problem - if I don't annotate the return value it also doesn't warn - and for that I don't need a delegate to break it:

[DAM(PublicMethods)]
Type _fieldType;

ref Type GetRefType() { return _fieldType; }

ref Type x = ref GetRefType();
x = typeof(Bar);
_fieldType.GetMethods();

This doesn't warn at all.

[vitek-karas]

In short it seems that we don't correctly handle ref return values.
And I didn't look at ref locals either (maybe there are issues there as well).

[MichalStrehovsky]

Ah yeah. Good catch. That one looks out of scope for this pull request.

[MichalStrehovsky]

We should make sure this also covers all the methods that we handle with intrinsics - last time I looked into it, not everything we handle intrinsically was properly annotated: dotnet/runtime#53121

[vitek-karas]

I'm not sure what we can do about this (other then going over the list once again and fixing the annotations).

The way it is now -> if there's an intrinsic handling which leads to warnings and the method has no annotation, it will not be caught.

If I change this to also look at all intrinsically recognized methods it would report on perfectly safe things, since we intrinsically handle patterns which do not produce any warnings.

I welcome any ideas here...

One idea:
Modify the switch in https://github.com/mono/linker/blob/4526d388102db2dc481522d9dee92b8f32414b23/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs#L304-L627 to return a boolen which indicates if the method should be handled here - meaning calling the method without further checks is potentially dangerous.

[MichalStrehovsky]

I would fix any of the necessary annotations in the runtime repo and not worry about anything else. Then we just add: "whenever an intrinsic is introduced, make sure it's annotated" into our list of things to look at when reviewing code.

Should be futureproof enough, including the scenarios of "using new illlink with an old framework".

[sbomer]

I'm not sure I understand why virtual methods are considered specially here. How would you override it at runtime without producing analysis warnings elsewhere, for example when instantiating the overriding type? We can't check annotations on any methods created at runtime so I would expect this to fall into the same bucket.

[vitek-karas]

The only way to override a method at runtime is via something like TypeBuilder which will need the MethodInfo of the base method to override. So we're guarding all the ways to get that MethodInfo - which is what this code does. The problem is that the other code in the app may call the base method with its annotation and assume that any type it gets as the return value will fulfill the annotation - but if that value comes from the runtime generated method, linker can't see it and validate - so we warn here.

Non-virtual methods can't be overriden, so there's no way (other than the ref return mentioned above) to change the implementation. Linker checks all implementations to validate that they fulfill the annotation requirements on return values - so no problem there. (Note that interface methods are also considered virtual here and the logic will apply to them as well).

[MichalStrehovsky]

It might help to add a blurb like this to the comment:

class Foo
{
    [return: DAM(Fields)]
    public abstract Type GetTypeWithFields();
}

Then we reflection emit a class that derives from Foo and add a virtual method named GetTypeWithFields with an implementation that is equivalent to return SomeOtherType.SomeMethod().

Since illink didn't see this, it wouldn't make sure whatever SomeMethod returns meets the requirements of the return type.

Our check ensures reflection.emit won't be able to derive from Foo without raising a warning (Ref.Emit requires the base type be annotated as DAM.All).

[vitek-karas]

I added a long description with a sample - hopefully it will make it a bit clearer.

[sbomer]

Thanks for the explanation! I didn't realize we were trying to support ref-emit use cases like this. By this logic shouldn't we warn on access to any virtual method, annotated or not? Otherwise someone could ref-emit a RUC override of an unannotated method and the caller of the base method would have no idea.

[vitek-karas]

We probably didn't do full analysis of this use case, but I would think that getting to a bad place through ref emit will be pretty hard (as in, we've done a good job guarding ourselves). The logic is basically that in order to call any method from a ref emit code, you must be able to reflection access that method (get a MethodInfo). All methods which lead to dangerous places should be annotated and thus should produce warnings when accessed through reflection. So even code which would use reflection from the ref emit code should still cause warnings, since it will have to get MethodInfo of the reflection APIs, which will produce warnings (since those APIs are annotated).

I think we have potential holes around things like out parameters, by ref values and so on - which is basically partially what this discussion is about, but ignoring those cases I can't think of another break (which means absolutely nothing... since I didn't realize the by ref problems either).