P/Invoke declaration lives outside a NativeMethods class

ID

vbnet.maintainability.move_p_invokes_to_native_methods_class

Severity

low

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Interop

Language

VB.NET

Description

Reports a P/Invoke declaration - a <DllImport> Function or Sub, or a legacy Declare statement - declared inside a type whose name is not NativeMethods, SafeNativeMethods or UnsafeNativeMethods. The convention these three names encode is what the check enforces: native declarations belong in a dedicated container, not alongside the business logic that calls them. Declarations already inside one of the three are not reported, whatever their accessibility.

Rationale

Where a native declaration lives determines how much of the code base has to be reviewed as unmanaged interop. A P/Invoke sitting among ordinary members hides two properties of that member from the reader: it can be called with no security demand once the containing type is trusted, and its correctness depends on marshalling that no compiler checks. Reviewers who audit a type for one reason do not usually audit it for the other, so a signature edited to add a parameter, or a CharSet left at its default, passes review in a file nobody was reviewing as interop code. Scattering also multiplies the declarations themselves: the same CreateFile or CancelJob gets declared in three classes with three slightly different signatures, and only one of them is correct on 64-bit. The three conventional names carry meaning that reviewers and analysis tools rely on - SafeNativeMethods for calls that are harmless to expose, UnsafeNativeMethods for calls that must not be, NativeMethods for everything that has not been classified - so grouping the declarations is also what makes it possible to state, in one place, which native calls the assembly makes at all.

The following code illustrates the pattern detected by this rule:

Public Sub Cancel(ByVal jobId As Integer)
    CancelJob(handle, jobId)
End Sub

' FLAGGED: P/Invoke declaration lives outside a NativeMethods class
<DllImport("winspool.drv", SetLastError:=True)>
Private Shared Function CancelJob(printer As IntPtr, job As Integer) As Boolean
End Function

Remediation

Move the declaration into a Friend NotInheritable Class NativeMethods with a private constructor, alongside the other native declarations of the assembly, and call it from a managed wrapper method that validates arguments and translates failures into exceptions. Use SafeNativeMethods for entry points that are safe for any caller to invoke and UnsafeNativeMethods for those that are not, so the distinction is visible from the call site. Keep one declaration per entry point, shared by every caller.

' Before: a native declaration among the business members of PrintSpooler
Public Class PrintSpooler
    <DllImport("winspool.drv", SetLastError:=True)>
    Private Shared Function CancelJob(printer As IntPtr, job As Integer) As Boolean
    End Function
End Class

' After
Friend NotInheritable Class NativeMethods
    Private Sub New()
    End Sub

    <DllImport("winspool.drv", SetLastError:=True)>
    Friend Shared Function CancelJob(printer As IntPtr, job As Integer) As Boolean
    End Function
End Class

Configuration

This detector does not need any configuration.

References