Roslyn Code analyzers for interop

[elinor-fung]

This tracking issue for a small backlog of rules around interop to add to the officially recommended set of analyzers. The rules are grouped around runtime functionality/features, but would represent separate analyzers in implementation. Since many issues concerning interop are rooted in a discrepancy between the user's intent and implementation, it is expected that only a small number of rules would be useful and applicable to all users. Official documentation will remain the main source of guidance for more nuanced behaviour that requires clear understanding of intent.

The upcoming plan to provide source generators for p/invokes should not directly affect the rules here, as that functionality would be using a different attribute for declaring p/invokes. However, there is the opportunity to provide an analyzer to help users migrate to the source generator approach. Once a source generator exists, the rules here could also be updated to consider p/invokes which use that generator or, depending on their severity, warnings within the generator itself.

The intent is that as the runtime interop team works through a backlog of analyzers around existing features and functionality, the concept of considering and adding analyzers for any new features will become natural and simply be part of that feature work. The P/Invokes and Marshalling rules are expected to be the starting points for working through this backlog.

P/Invokes

These rules inspect the actual declaration and invocation of p/invokes.

• Do not use [Out] string for P/Invokes: Do not use [Out] string for P/Invokes #35692
• Avoid StringBuilder parameters for P/Invokes: Avoid StringBuilder parameters for P/Invokes #35693
• Prefer ExactSpelling=true in [DllImport] for known APIs: Prefer ExactSpelling=true on [DllImport] for known APIs #35695
• Remove redundant configuration from [DllImport] declaration Remove redundant configuration from [DllImport] declaration #33808
• CA1404 port: Port FxCop rule CA1404: CallGetLastErrorImmediatelyAfterPInvoke roslyn-analyzers#420
• Prefer SafeHandle over IntPtr for known APIs: Analyzer suggestion: Prefer SafeHandle over IntPtr #42404
• Get the last error after calls to P/Invokes with SetLastError=true
  • Category: Interoperability
  • Default: Enabled
  • After calling a p/invoke that sets the last error, Marshal.GetLastWin32Error should be called to retrieve the last error.

    [DllImport("MyLibrary", SetLastError = true)]
    private static extern void Foo();
    
    public static void Bar()
    {
        Foo(); // Flag last error not retrieved
    }
    
    public static void Baz()
    {
        Foo(); // OK
        if (Marshal.GetLastWin32Error() == 0) { ... }
    }

• Specify SetLastError=true in [DllImport] for known APIs
  • Category: Interoperability
  • Default: Enabled
  • For APIs that are known to set last error, SetLastError=true should be specified. This would require having/building a database of APIs to compare against.

    // Flag SetLastError=false for known API
    [DllImport("kernel32")]
    public static extern bool CloseHandle(IntPtr hObject);
    
    // OK - known API does not set last error
    [DllImport("kernel32")]
    public static extern int GetCurrentProcessId();

  • Recommend:

    [DllImport("kernel32.dll", SetLastError = true)]
    public static extern bool CloseHandle(IntPtr hObject);

  • If the user does not intend to check the error, this can be skipped, but the general guidance is to both define the p/invoke to set last error (for APIs that do so) and check the error.

Marshalling

These rules inspect how data is marshalled. They would apply to the parameters on p/invokes and any types (e.g. marshalled structs, delegates) used by p/invokes.

• Specify SizeConst when marshalling as ByValArray: Specify SizeConst when marshalling field as ByValArray #36134
• CA1414 port: Port FxCop rule CA1414: MarkBooleanPInvokeArgumentsWithMarshalAs roslyn-analyzers#430
• Avoid Delegate or MulticastDelegate fields in marshalled structs
  • Category: Interoperability
  • Default: Enabled
  • Delegate and MulticastDelegate do not have a required signature, so they do not guarantee that the delegate passed in will match the signature the native code expects. Marshalling a struct containing a Delegate or MulticastDelegate from its native representation to a managed object can destabilize the runtime if the value of the field in the native representation is not a function pointer that wraps a managed delegate. Use a specific delegate type instead of Delegate or MulticastDelegate.

    [DllImport("MyLibrary")]
    private static extern void Foo(MyStruct s);
    
    struct MyStruct
    {
        Delegate Bar; // Flag Delegate in marshalled struct
    }

• Rules for implementing ICustomMarshaler
  • GetInstance() requirement - Analyzer proposal: Missing GetInstance for ICustomMarshalers #46521

COM

These rules inspect COM-related functionality. Many of the existing rules that have not yet been ported are around COM.

• Types should not be both [ComImport] and [ComVisible(true)]
  • Types should not be marked as both imported from COM and made visible to COM.
• COM-visible types should be accessible and creatable
  • Types marked ComVisible(true) should be public, non-abstract, and have a public parameterless constructor.
  • Port FxCop rule CA1409: ComVisibleTypesShouldBeCreatable roslyn-analyzers#425
    • CA1409 looks to have the same intent, but only specifically flagged the public parameterless constructor
• Partial interface definitions with ComImport – Warning for partial ComImport types with members in more than one type part #59013.

Low-level interop APIs

These rules are related to APIs that provide low-level interaction/integration with the runtime's interop system.

• Interfaces marked with DynamicInterfaceCastableImplementationAttribute should provide a default implementation of all inherited interface methods: All inherited interface members should be implemented on an interface marked with DynamicInterfaceCastableImplementationAttribute #41529

Cross-platform

There is an existing proposal for an analyzer that detects the use of platform-specific APIs where the API might not be available. Many interop-related APIs are platform-specific and would be given the appropriate attribute such that the proposed analyzer would flag their use.

The logic that will be used by the proposed analyzer for platform-specific APIs could be leveraged for platform-specifc interop behaviour that is not tied to a specifid API. There are some types or MarshalAs values for which marshalling is not supported on all platforms. In these cases, it is not a known API that is platform-specific, but a user-defined p/invoke that is platform-specific due to the way it is defined. An analyzer would detect those p/invokes and treat those as platform-specific calls, following the same logic as that for the platform-specific APIs for determining the platform context and checking for platform guards around the call site.

Rules:

• Marshalling of <Type> requires <OS>
• Marshalling as <UnmanagedType.*> requires <OS>

Existing rules

There are a number of rules around interop and marshalling that go through legacy (static) analysis. They have all previously been deprecated or slated to be ported as time allows. See FxCop rule port status for a list of all ported, tracked, and deprecated rules.

[elinor-fung]

@AaronRobinsonMSFT @jkoritzinsky

[rseanhall]

In most Win32 APIs, it is wrong to use Marshal.GetLastWin32Error to check if the API failed because most functions that set the thread's last-error code [only] set it when they fail.

[elinor-fung]

That is definitely the case for the Win32 APIs themselves. However, when going through a p/invoke in .NET Core with SetLastError=true, the runtime will explicitly clear the last error before calling the target function: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/dllimport.cpp#L862-L868

[rseanhall]

Interesting, I didn't know that. It doesn't really solve the problem though. CreateFile is a typical Win32 API. In its documentation, it states how to tell when there's a failure (the returned value is INVALID_HANDLE_VALUE). Then it says if it failed, call GetLastError for more information. Nowhere does it say that GetLastError will return 0 on success. So theoretically, CreateFile could internally call another API which sets the last error to non-zero, and still complete successfully without ever putting it back to zero while still adhering to the documented contract.

This code relies on undocumented behavior:

var hFile = CreateFile(...);
if (Marshal.GetLastWin32Error() == 0)
{
  //succeeded
}
else
{
  //failed
}

This code doesn't directly check the last error but is perfectly valid (Win32Exception calls Marshal.GetLastWin32Error internally):

var hFile = CreateFile(...);
if (hFile == INVALID_HANDLE_VALUE)
{
  throw new Win32Exception();
}

[elinor-fung]

The spirit of the rule is that there is a path after the p/invoke where the last error is retrieved without another call in between. As your example points out, only expecting Marshal.GetLastWin32Error is not sufficient, but I believe that still fits within the intent of the rule (i.e. the rule should consider Win32Exception and not flag something like that).

[Mrnikbobjeff]

One issue which seems to have fallen through the tracks seems to be #33808 which is not referenced in the todo list if I read this correctly

[elinor-fung]

Thanks @Mrnikbobjeff - updated the list.

[Mrnikbobjeff]

Are these rules already approved when they are in the list? Would a PR be accepted for them or do they individually require approval?

[AaronRobinsonMSFT]

@Mrnikbobjeff Great question. This issue is for tracking interop analyzers as an initiative and doesn't represent specific approval for any of them. Each analyzer requires a separate API review via a new issue. Some however have had API reviews and been approved - see #35695 for "Prefer ExactSpelling=true in [DllImport] for known APIs". When the API review is completed and marked with the api-approved tag the analyzer can be implemented by anyone. See previously completed analyzer issues above and refer to the associated PR for the repo and process.

If so incline, anyone in the community can submit a PR for the existing approved but not completed analyzer above (#35695) and/or submit new issues marked with api-suggestion and begin the review process. If submitting a new PR, please let us know here and tag one of us (@elinor-fung, @jkoritzinsky, or myself) on the issue so we can update this issue and help the process along.

[Mrnikbobjeff]

For the GetLastError heuristic some things are not clear to me. How would one arrive at a list of methods calling setlasterror. I thought about how other languages might approach this, as I had already written a Java bytecode analyzer which finds method calls inside of a function. Build the list up transitively and one would know which methods call SetLastError. As this is not viable in ASM and SetLastError is documented on a per function basis I have no idea on creating the desired list. Are there any approaches already considered?

[AaronRobinsonMSFT]

@Mrnikbobjeff Unfortunately there isn't. Any Win32 API could call another Win32 API that call SetLastError() - this means even if a function doesn't call it directly it can be called. One could argue that any Win32 API that returns a non-error code (i.e. BOOL or handle type) is implicitly going to call SetLastError() in order to provide more detailed information.

The TL;DR of the hand-wavy statements above is the only way to "know" is to follow the official documentation or guess based on the function signature.

[carlossanlop]

@elinor-fung Looks like this other approved proposal belongs to this list:
#51193

[elinor-fung]

@carlossanlop This issue is listing out a backlog of interop-related analyzers. That one looks to be more general performance. I don't believe we do a tracking list for general runtime/performance analyzers.