Verify if COM dangerous during call site check

jkurdek commented 2 years ago

The motivation of the change is to reduce the number of unnecessary warnings around PInvokes. The change makes the RequiresReflectionMethodBodyScannerForCallSite check for COM to decide whether dataflow analysis is needed for PInvokes.

MichalStrehovsky commented 2 years ago

The motivation of the change is to reduce the number of unnecessary warnings around PInvokes.

What unnecessary warnings does this remove?

If we go to the lengths of actually figuring out this is unsafe COM, we might as well just generate the warning and not run the dataflow.

It was written like this because the rules for COM are actually more complex - we just don't detect them all right now. E.g. we don't detect:

unsafe class Program
{
    [StructLayout(LayoutKind.Sequential)]
    class Base
    { }

    [StructLayout(LayoutKind.Sequential)]
    class Derived : Base
    {
        public object O;
    }

    [DllImport("...")]
    static extern void Foo([MarshalAs(UnmanagedType.LPStruct)] Base a);

    unsafe static void Main()
    {
        Foo(new Derived());
    }
}

vitek-karas commented 2 years ago

If the method in question is a compiler generated method which is accessed via reflection (typically due to DAM applied to the type with private reflection ability), we don't run data flow on the compiler generated method (we don't have enough context to do that correctly), instead we warn that there's reflection access to something which looks problematic.

In this case, any compiler generated method with PInvoke call will trigger the warning currently - regardless if the PInvoke is problematic or not.

vitek-karas commented 2 years ago

I agree that this is not perfect - but this change is solely about the compiler generated code interaction.

dotnet / linker

Verify if COM dangerous during call site check #3009