Potential bug found by hamarb123.Analyzers (com wrappers)

[hamarb123]

After realizing hamarb123.Analyzers could have found both instances in this issue: #129664, I decided to just run it on all of dotnet/runtime (at least, clr+libs+libs.tests subsets). Here's another bug it found:

• /src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs(713,21): warning HAM0001: Call to member 'Free' will execute on a defensive copy of '_nativeObjectWrapperWeakHandle'

We can end up double free-ing due to this, as we can create here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Lines 1223 to 1229 in e556d5a

	NativeObjectWrapper nativeObjectWrapper = NativeObjectWrapper.Create(
	identity,
	inner,
	this,
	comProxy,
	flags,
	ref referenceTrackerMaybe);

Which can instantiate a ReferenceTrackerNativeObjectWrapper here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Line 583 in e556d5a

return new ReferenceTrackerNativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags, trackerObject);

And potentially call .Release() here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Line 1241 in e556d5a

nativeObjectWrapper.Release();

But never suppress GC finalization, so the finalizer can run:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Line 662 in e556d5a

Release();

Which calls this a second time:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Lines 707 to 719 in e556d5a

	public override void Release()
	{
	// Remove the entry from the cache that keeps track of the active NativeObjectWrappers.
	if (_nativeObjectWrapperWeakHandle.IsAllocated)
	{
	TrackerObjectManager.s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle);
	_nativeObjectWrapperWeakHandle.Free();
	}
	DisconnectTracker();
	base.Release();
	}

But the field is readonly and thus we seemingly (unless there's something unobvious stopping it from being possible) double-free in this race-condition handling as Free() runs on a defensive copy:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Line 672 in e556d5a

internal readonly GCHandle _nativeObjectWrapperWeakHandle;

It is possible however that one of the checks like nativeObjectWrapper.IsUniqueInstance implicitly stops this from being possible somehow and I'm not familiar with this code so I don't really know, hence why it's a potential bug.

/cc @jkotas @jkoritzinsky

(The rest of stuff found by hamarb123.Analyzers either didn't look like a bug at a glance, wasn't a bug after checking (although there's one that should maybe be fixed anyway), or was globalization related (and thus I couldn't be bothered to investigate its validity at all). Lmk if you want me to post all results from it somewhere.)

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.