Warn when accessing DAM annotated method via reflection #2145
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1557,6 +1557,23 @@ void MarkField (FieldDefinition field, in DependencyInfo reason) | |
| Annotations.Mark (field, reason); | ||
| } | ||
|
|
||
| switch (reason.Kind) { | ||
| case DependencyKind.AccessedViaReflection: | ||
| case DependencyKind.DynamicDependency: | ||
| case DependencyKind.DynamicallyAccessedMember: | ||
| case DependencyKind.InteropMethodDependency: | ||
| if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field) && | ||
| !ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ()) | ||
| _context.LogWarning ( | ||
|
There was a problem hiding this comment. Do we need a call to ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode? There was a problem hiding this comment. Great catch - thanks a lot! |
||
| $"Field '{field.GetDisplayName ()}' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", | ||
| 2110, | ||
| _scopeStack.CurrentScope.Origin, | ||
| MessageSubCategory.TrimAnalysis); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
|
|
||
| if (CheckProcessed (field)) | ||
| return; | ||
|
|
||
|
|
@@ -2711,12 +2728,12 @@ protected virtual MethodDefinition MarkMethod (MethodReference reference, Depend | |
|
|
||
| // Use the original reason as it's important to correctly generate warnings | ||
| // the updated reason is only useful for better tracking of dependencies. | ||
| ProcessAnalysisAnnotationsForMethod (method, originalReasonKind); | ||
|
|
||
| return method; | ||
| } | ||
|
|
||
| void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKind dependencyKind) | ||
| { | ||
| switch (dependencyKind) { | ||
| // DirectCall, VirtualCall and NewObj are handled by ReflectionMethodBodyScanner | ||
|
|
@@ -2773,24 +2790,85 @@ void ProcessRequiresUnreferencedCode (MethodDefinition method, DependencyKind de | |
| return; | ||
|
|
||
| default: | ||
| // All other cases have the potential of us missing a warning if we don't report it | ||
| // It is possible that in some cases we may report the same warning twice, but that's better than not reporting it. | ||
| break; | ||
| } | ||
|
|
||
| // All override methods should have the same annotations as their base methods | ||
| // (else we will produce warning IL2046 or IL2092 or some other warning). | ||
| // When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method. | ||
| if (dependencyKind == DependencyKind.DynamicallyAccessedMember && | ||
| method.IsVirtual && | ||
| Annotations.GetBaseMethods (method) != null) | ||
| return; | ||
|
|
||
| CheckAndReportRequiresUnreferencedCode (method); | ||
|
|
||
| if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (method)) { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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: There was a problem hiding this comment. 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". |
||
| // If the current scope has analysis warnings suppressed, don't generate any | ||
| if (ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ()) | ||
| return; | ||
|
|
||
| // ReflectionMethodBodyScanner handles more cases for data flow annotations | ||
| // so don't warn for those. | ||
| switch (dependencyKind) { | ||
| case DependencyKind.AttributeConstructor: | ||
| case DependencyKind.AttributeProperty: | ||
| return; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
|
|
||
| // If the method only has annotation on the return value and it's not virtual avoid warning. | ||
| // Return value annotations are "consumed" by the caller of a method, and as such there is nothing | ||
| // wrong calling these dynamically. The only problem can happen if something overrides a virtual | ||
| // method with annotated return value at runtime - in this case the trimmer can't validate | ||
| // that the method will return only types which fulfill the annotation's requirements. | ||
|
Comment on lines
+2824
to
+2828
There was a problem hiding this comment. We should also disallow this if the return value is a There was a problem hiding this comment. 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?There was a problem hiding this comment. 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);There was a problem hiding this comment. But There was a problem hiding this comment.
There was a problem hiding this comment. I see - the magical line is: _typeField.GetMethods(); // This doesn't warn, but it now contains typeof(Bar) which nobody checkedThere was a problem hiding this comment. 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. There was a problem hiding this comment. In short it seems that we don't correctly handle ref return values. There was a problem hiding this comment. Ah yeah. Good catch. That one looks out of scope for this pull request. There was a problem hiding this comment. 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. There was a problem hiding this comment. The only way to override a method at runtime is via something like 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). There was a problem hiding this comment. 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 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). There was a problem hiding this comment. I added a long description with a sample - hopefully it will make it a bit clearer. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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). |
||
| // For example: | ||
| // class BaseWithAnnotation | ||
| // { | ||
| // [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] | ||
| // public abstract Type GetTypeWithFields(); | ||
| // } | ||
| // | ||
| // class UsingTheBase | ||
| // { | ||
| // public void PrintFields(Base base) | ||
| // { | ||
| // // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value. | ||
| // Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name))); | ||
| // } | ||
| // } | ||
| // | ||
| // If at runtime (through ref emit) something generates code like this: | ||
| // class DerivedAtRuntimeFromBase | ||
| // { | ||
| // // No point in adding annotation on the return value - nothing will look at it anyway | ||
| // // Linker will not see this code, so there are no checks | ||
| // public override Type GetTypeWithFields() { return typeof(TestType); } | ||
| // } | ||
| // | ||
| // If TestType from above is trimmed, it may note have all its fields, and there would be no warnings generated. | ||
| // But there has to be code like this somewhere in the app, in order to generate the override: | ||
| // class RuntimeTypeGenerator | ||
| // { | ||
| // public MethodInfo GetBaseMethod() | ||
| // { | ||
| // // This must warn - that the GetTypeWithFields has annotation on the return value | ||
| // return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields"); | ||
| // } | ||
| // } | ||
| if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method)) | ||
| return; | ||
|
|
||
| _context.LogWarning ( | ||
| $"Method '{method.GetDisplayName ()}' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.", | ||
| 2111, | ||
| _scopeStack.CurrentScope.Origin, | ||
| MessageSubCategory.TrimAnalysis); | ||
| } | ||
| } | ||
|
|
||
| internal bool ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode () | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree, but it would require non-trivial effort. Not for fields, but for methods. The entire RUC and DAM detection in
MarkMethodcurrently 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
MarkMethodwould 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 andDependencyInfointo one parameter. With that we could relatively easily add more fields to the structure we will pass around and store additional information that way.