Skip to content

Verify if COM dangerous during call site check#3009

Merged
vitek-karas merged 2 commits intodotnet:mainfrom
jkurdek:report-less-warnings-for-pinvokes
Aug 31, 2022
Merged

Verify if COM dangerous during call site check#3009
vitek-karas merged 2 commits intodotnet:mainfrom
jkurdek:report-less-warnings-for-pinvokes

Conversation

@jkurdek
Copy link
Contributor

@jkurdek jkurdek commented Aug 30, 2022

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.

@jkurdek jkurdek requested review from sbomer and vitek-karas August 30, 2022 13:21
@jkurdek jkurdek self-assigned this Aug 30, 2022
@jkurdek jkurdek requested a review from marek-safar as a code owner August 30, 2022 13:21
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

The change looks good - would you mind adding a testcase? Thanks!

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky
Copy link
Member

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());
    }
}

Related: dotnet/runtime#74697

@vitek-karas
Copy link
Member

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
Copy link
Member

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

@vitek-karas vitek-karas merged commit 2177386 into dotnet:main Aug 31, 2022
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
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.

Commit migrated from dotnet/linker@2177386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants