Add missing GC.KeepAlives to WMIInterop#124796
Conversation
| @@ -270,6 +271,8 @@ public int PutMethod_(string wszName, int lFlags, IWbemClassObjectFreeThreaded p | |||
| if (pWbemClassObject == IntPtr.Zero) | |||
| throw new ObjectDisposedException(name); | |||
| int res = WmiNetUtilsHelper.PutMethod_f(20, pWbemClassObject, wszName, lFlags, pInSignature, pOutSignature); | |||
There was a problem hiding this comment.
It may look like we pass objects directly here, but it's just an overriden operator:
internal sealed class IWbemClassObjectFreeThreaded : IDisposable
{
private IntPtr pWbemClassObject = IntPtr.Zero;
public static implicit operator IntPtr(IWbemClassObjectFreeThreaded wbemClassObject)
{
if (null == wbemClassObject)
return IntPtr.Zero;
return wbemClassObject.pWbemClassObject;
}|
Tagging subscribers to this area: @dotnet/area-system-management |
There was a problem hiding this comment.
Pull request overview
This PR addresses potential GC safety issues in the System.Management WMI interop layer by adding GC.KeepAlive calls for objects whose IntPtr fields are passed to native code. The IWbemClassObjectFreeThreaded class wraps native COM object pointers and has an implicit conversion operator to IntPtr that extracts the internal pWbemClassObject field. Without keeping the wrapper object alive, the GC could prematurely collect it during the native call, potentially invalidating the pointer.
Changes:
- Added
GC.KeepAlive(pCompareTo)inCompareTo_method to keep the comparison object alive during the native comparison call - Added
GC.KeepAlive(pInSignature)andGC.KeepAlive(pOutSignature)inPutMethod_to keep method signature objects alive during the native PutMethod call
|
PTAL @jkotas @AaronRobinsonMSFT @jkoritzinsky to validate my (and Opus's) assumptions |
I would agree with your and Opus's analysis. The fact that the pointer is being passed "by value" makes this look correct, but since the containing object has a finalizer that will free that pointer means it is possible this results in a "use after free" failure. The interop in this file looks baroque and a bit outdated, unsurprising given it is WMI goo. I'm sure rewriting it using modern principles and patterns would make it less sketchy.
This appears to be another version of basically the same problem :( |
Opus highlighted these as potential issues.
We pass IntPtr fields to native callbacks, but we don't root the object holding them (it's possible that it's implicitly is done by GC.KeepAlive(this), but I wasn't able to prove that).