Call to a framework method whose behaviour is unreliable or unsafe

ID

vbnet.correctness.avoid_calling_problematic_methods

Severity

critical

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

medium

Resource

Api Design

Language

VB.NET

Description

Reports calls to a small set of framework methods that are documented as problematic and have a safer replacement: GC.Collect, Thread.Suspend and Thread.Resume, AppDomain.Unload, Assembly.LoadFrom, and SafeHandle.DangerousGetHandle. Each is recognised by the method name together with the receiver it is called on - GC, Assembly and AppDomain qualifiers, and a variable whose name identifies a thread for Suspend and Resume - so a same-named method on an unrelated type is not reported. Related methods that are not problematic, such as Assembly.Load, GC.WaitForPendingFinalizers and AppDomain.CreateDomain, are left alone. Test sources are excluded, since forcing a collection is a legitimate step in a memory test.

Rationale

These calls fail in ways that are hard to attribute back to them. GC.Collect forces a blocking collection of every generation, discarding the collector’s own tuning: the long-lived objects it was deliberately leaving in generation 0 get promoted, so the next natural collection has more to do, and the pattern typically makes throughput and pause times worse than the behaviour it was added to fix. Thread.Suspend stops a thread at an arbitrary instruction, which may be inside a lock, a finally block or the allocator; the suspended thread keeps whatever it holds, and any thread that later needs it deadlocks - which is why the methods are obsolete and unavailable on .NET Core. AppDomain.Unload cannot force a thread out of the domain being torn down: a thread in an unbreakable region or a finalizer that does not return leaves the unload hanging or raises CannotUnloadAppDomainException, and the caller has no way to tell which. Assembly.LoadFrom resolves through a separate load context with its own binding rules, so the same file can end up loaded twice under different identities; the types then compare unequal, casts between them fail with messages naming the same type on both sides, and a dependency next to the loaded file wins over the one the application resolved. SafeHandle.DangerousGetHandle returns the raw handle with no reference held, so the SafeHandle may be finalized while the returned value is still in use - the handle is then closed, or worse recycled by the operating system for something else, and the call that uses it operates on an unrelated object.

The following code illustrates the pattern detected by this rule:

Public Sub TrimMemory()
    ' FLAGGED: Call to a framework method whose behaviour is unreliable or unsafe
    GC.Collect()
    ' FLAGGED: Call to a framework method whose behaviour is unreliable or unsafe
    System.GC.Collect(2, GCCollectionMode.Forced)

Remediation

Replace each call with the supported alternative. Let the garbage collector schedule its own collections and, where a specific object must be released promptly, implement IDisposable and use Using instead of forcing a collection. Coordinate threads with a ManualResetEventSlim, SemaphoreSlim or a CancellationToken that the thread itself observes, rather than suspending it from outside. Isolate untrusted or reloadable code in a separate process, or on .NET Core in a collectible AssemblyLoadContext, instead of relying on domain unload. Load assemblies by identity with Assembly.Load so binding stays in one context. Pass the SafeHandle itself to the P/Invoke declaration and let the marshaller keep it alive; if the raw value is unavoidable, bracket its use with DangerousAddRef and DangerousRelease.

' Before
Public Sub Pause()
    workerThread.Suspend()
    GC.Collect()
End Sub

' After
Public Sub Pause()
    pauseRequested.Reset()   ' the worker waits on this at a safe point
End Sub

Configuration

This detector does not need any configuration.

References