Skip to content

Warn when accessing DAM annotated method via reflection #2145

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.

Sign up for GitHub

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

Merged
sbomer merged 6 commits into from
Jul 29, 2021
Merged

Warn when accessing DAM annotated method via reflection #2145

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dee0419
Warn when accessing DAM annotated method via reflection
vitek-karas Jun 23, 2021
4ddd3ed
Formatting
vitek-karas Jul 14, 2021
e640a9e
Only warn on virtual methods with annotate return value
vitek-karas Jul 15, 2021
ac215d3
PR feedback
vitek-karas Jul 22, 2021
5d180d5
PR feedback
vitek-karas Jul 26, 2021
23a62ac
Remove test code.
vitek-karas Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
PR feedback
  • Loading branch information
@vitek-karas
vitek-karas committed Jul 26, 2021
commit 5d180d56d08e699935c7ee3f411d705bb82a0e68
34 changes: 34 additions & 0 deletions src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2826,6 +2826,40 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin
// 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.
Copy link
Member

@sbomer sbomer Jul 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@vitek-karas vitek-karas Jul 26, 2021

Choose a reason for hiding this comment

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

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).

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jul 26, 2021

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@vitek-karas vitek-karas Jul 26, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@sbomer sbomer Jul 26, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@vitek-karas vitek-karas Jul 26, 2021

Choose a reason for hiding this comment

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

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;

Expand Down