From 917b61c980e2f664b2af769d433d9299494772f4 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 5 Mar 2021 10:07:26 -0800 Subject: [PATCH 1/7] Remove unnecessary AddRef(). Found during new ComWrappers aggregation API in .NET 6. --- src/coreclr/src/interop/interoplib.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/src/interop/interoplib.cpp b/src/coreclr/src/interop/interoplib.cpp index 9aff8c2bb335c1..3693e6b2086938 100644 --- a/src/coreclr/src/interop/interoplib.cpp +++ b/src/coreclr/src/interop/interoplib.cpp @@ -73,8 +73,6 @@ namespace InteropLib if (mow == nullptr) return E_INVALIDARG; - (void)mow->AddRef(); - *object = mow->Target; return S_OK; } From 068ecdeea8a051e31cf4b7a36dd9fc0c16167400 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 9 Apr 2021 20:35:22 -0700 Subject: [PATCH 2/7] Reference Tracker runtime can QI when object is collected. It is possible that a Reference Tracker runtime can QI a managed object wrapper when it has been marked as Destroyed or the associated managed object has been collected. The QI impl must safely ensure prior to performing the QI that the GC hasn't collected the wrapped managed object. --- src/coreclr/src/interop/comwrappers.cpp | 43 ++++++++++++++++++- src/coreclr/src/interop/comwrappers.hpp | 5 +++ .../src/interop/inc/interoplibimports.h | 3 ++ .../src/interop/trackerobjectmanager.cpp | 4 +- src/coreclr/src/vm/interoplibinterface.cpp | 25 +++++++++++ 5 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/interop/comwrappers.cpp b/src/coreclr/src/interop/comwrappers.cpp index f39472778cdb91..7307f73aa4fa50 100644 --- a/src/coreclr/src/interop/comwrappers.cpp +++ b/src/coreclr/src/interop/comwrappers.cpp @@ -4,6 +4,7 @@ #include "comwrappers.hpp" #include #include +#include #include // placement new @@ -188,7 +189,7 @@ HRESULT STDMETHODCALLTYPE ManagedObjectWrapper_QueryInterface( /* [iid_is][out] */ _COM_Outptr_ void __RPC_FAR* __RPC_FAR* ppvObject) { ManagedObjectWrapper* wrapper = ABI::ToManagedObjectWrapper(disp); - return wrapper->QueryInterface(riid, ppvObject); + return wrapper->QueryInterface(disp, riid, ppvObject); } namespace @@ -238,6 +239,11 @@ namespace return static_cast(c & ComRefCountMask); } + constexpr bool IsMarkedToDestroy(_In_ ULONGLONG c) + { + return (c & DestroySentinel) != 0; + } + ULONG STDMETHODCALLTYPE TrackerTarget_AddRefFromReferenceTracker(_In_ ABI::ComInterfaceDispatch* disp) { _ASSERTE(disp != nullptr && disp->vtable != nullptr); @@ -458,9 +464,14 @@ ManagedObjectWrapper::ManagedObjectWrapper( , _dispatches{ dispatches } , _refCount{ 1 } , _flags{ flags } + , _refTrackerDispatch{ nullptr } { bool wasSet = TrySetObjectHandle(objectHandle); _ASSERTE(wasSet); + + // Retain the dispatch entry for the IReferenceTrackerTarget so we + // can branch quickly during a QI from that interface. + _refTrackerDispatch = (const ABI::ComInterfaceDispatch*)AsRuntimeDefined(__uuidof(IReferenceTrackerTarget)); } ManagedObjectWrapper::~ManagedObjectWrapper() @@ -542,6 +553,11 @@ bool ManagedObjectWrapper::IsRooted() const return rooted; } +bool ManagedObjectWrapper::IsMarkedToDestroy() const +{ + return ::IsMarkedToDestroy(_refCount); +} + ULONG ManagedObjectWrapper::AddRefFromReferenceTracker() { LONGLONG prev; @@ -595,12 +611,37 @@ HRESULT ManagedObjectWrapper::Unpeg() } HRESULT ManagedObjectWrapper::QueryInterface( + _In_ const ABI::ComInterfaceDispatch* dispatch, /* [in] */ REFIID riid, /* [iid_is][out] */ _COM_Outptr_ void __RPC_FAR* __RPC_FAR* ppvObject) { if (ppvObject == nullptr) return E_POINTER; + // Check if this is a QI from an IReferenceTrackerTarget. + ComHolder releaseMaybe; + if (dispatch == _refTrackerDispatch) + { + // AddRef to keep the managed object alive. + // AddRef is "safe" at this point because if it is a MOW with outstanding + // Reference Tracker reference, we know for sure the MOW is not claimed yet + // but the managed object could be. + AddRef(); + + // We are taking an extra AddRef() that now must be released. + releaseMaybe.Attach(this); + + // For MOWs that have outstanding Reference Tracker reference, they could be either: + // 1. Marked to Destroy - in this case it is unsafe to touch wrapper. + // 2. Object Handle target has been NULLed out by GC. + if (IsMarkedToDestroy() || !InteropLibImports::HasValidTarget(Target)) + { + // It is unsafe to proceed with a QueryInterface call. The MOW has been + // marked destroyed or the associated managed object has been collected. + return COR_E_ACCESSING_CCW; + } + } + // Find target interface *ppvObject = AsRuntimeDefined(riid); if (*ppvObject == nullptr) diff --git a/src/coreclr/src/interop/comwrappers.hpp b/src/coreclr/src/interop/comwrappers.hpp index e4d849a562574a..ef5ffa7dfdc9bd 100644 --- a/src/coreclr/src/interop/comwrappers.hpp +++ b/src/coreclr/src/interop/comwrappers.hpp @@ -47,6 +47,7 @@ class ManagedObjectWrapper LONGLONG _refCount; Volatile _flags; + const ABI::ComInterfaceDispatch* _refTrackerDispatch; public: // static // Get the implementation for IUnknown. @@ -101,6 +102,9 @@ class ManagedObjectWrapper // Indicate if the wrapper should be considered a GC root. bool IsRooted() const; + // Check if the wrapper has been marked to be destroyed. + bool IsMarkedToDestroy() const; + public: // IReferenceTrackerTarget ULONG AddRefFromReferenceTracker(); ULONG ReleaseFromReferenceTracker(); @@ -109,6 +113,7 @@ class ManagedObjectWrapper public: // Lifetime HRESULT QueryInterface( + _In_ const ABI::ComInterfaceDispatch* dispatch, /* [in] */ REFIID riid, /* [iid_is][out] */ _COM_Outptr_ void __RPC_FAR * __RPC_FAR * ppvObject); ULONG AddRef(void); diff --git a/src/coreclr/src/interop/inc/interoplibimports.h b/src/coreclr/src/interop/inc/interoplibimports.h index deb3f196f96c2c..57824c36d78caa 100644 --- a/src/coreclr/src/interop/inc/interoplibimports.h +++ b/src/coreclr/src/interop/inc/interoplibimports.h @@ -44,6 +44,9 @@ namespace InteropLibImports // Delete Object instance handle. void DeleteObjectInstanceHandle(_In_ InteropLib::OBJECTHANDLE handle) noexcept; + // Check if Object instance handle still points at an Object. + bool HasValidTarget(_In_ InteropLib::OBJECTHANDLE handle) noexcept; + // Get the current global pegging state. bool GetGlobalPeggingState() noexcept; diff --git a/src/coreclr/src/interop/trackerobjectmanager.cpp b/src/coreclr/src/interop/trackerobjectmanager.cpp index f205484d3b0af6..658143ce02719e 100644 --- a/src/coreclr/src/interop/trackerobjectmanager.cpp +++ b/src/coreclr/src/interop/trackerobjectmanager.cpp @@ -176,8 +176,8 @@ namespace ManagedObjectWrapper* mow = ManagedObjectWrapper::MapFromIUnknown(target); - // Not a target we implemented. - if (mow == nullptr) + // Not a target we implemented or wrapper is marked to be destroyed. + if (mow == nullptr || mow->IsMarkedToDestroy()) return S_OK; // Notify the runtime a reference path was found. diff --git a/src/coreclr/src/vm/interoplibinterface.cpp b/src/coreclr/src/vm/interoplibinterface.cpp index ce181ce9ccd103..75ec59d35a2a64 100644 --- a/src/coreclr/src/vm/interoplibinterface.cpp +++ b/src/coreclr/src/vm/interoplibinterface.cpp @@ -1024,6 +1024,31 @@ namespace InteropLibImports DestroyHandleCommon(static_cast<::OBJECTHANDLE>(handle), InstanceHandleType); } + bool HasValidTarget(_In_ InteropLib::OBJECTHANDLE handle) noexcept + { + CONTRACTL + { + NOTHROW; + MODE_PREEMPTIVE; + PRECONDITION(handle != NULL); + } + CONTRACTL_END; + + bool isValid = false; + ::OBJECTHANDLE objectHandle = static_cast<::OBJECTHANDLE>(handle); + + HRESULT hr = S_OK; + BEGIN_EXTERNAL_ENTRYPOINT(&hr) + { + // Switch to cooperative mode so the handle can be safely inspected. + GCX_COOP(); + isValid = ObjectFromHandle(objectHandle) != NULL; + } + END_EXTERNAL_ENTRYPOINT; + + return isValid; + } + bool GetGlobalPeggingState() noexcept { CONTRACTL From e24ac53e34bd7841a5b300debb97067ee04524db Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 9 Apr 2021 20:41:07 -0700 Subject: [PATCH 3/7] Set the global pegging state in the same order as .NET Framework. --- src/coreclr/src/interop/trackerobjectmanager.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/interop/trackerobjectmanager.cpp b/src/coreclr/src/interop/trackerobjectmanager.cpp index 658143ce02719e..ff5fcdb05c9425 100644 --- a/src/coreclr/src/interop/trackerobjectmanager.cpp +++ b/src/coreclr/src/interop/trackerobjectmanager.cpp @@ -331,10 +331,6 @@ HRESULT TrackerObjectManager::BeginReferenceTracking(_In_ RuntimeCallContext* cx s_HasTrackingStarted = TRUE; - // From this point, the tracker runtime decides whether a target - // should be pegged or not as the global pegging flag is now off. - InteropLibImports::SetGlobalPeggingState(false); - // Let the tracker runtime know we are about to walk external objects so that // they can lock their reference cache. Note that the tracker runtime doesn't need to // unpeg all external objects at this point and they can do the pegging/unpegging. @@ -342,6 +338,10 @@ HRESULT TrackerObjectManager::BeginReferenceTracking(_In_ RuntimeCallContext* cx _ASSERTE(s_TrackerManager != nullptr); RETURN_IF_FAILED(s_TrackerManager->ReferenceTrackingStarted()); + // From this point, the tracker runtime decides whether a target + // should be pegged or not as the global pegging flag is now off. + InteropLibImports::SetGlobalPeggingState(false); + // Time to walk the external objects RETURN_IF_FAILED(WalkExternalTrackerObjects(cxt)); From 8115132fbd43fdace6c596f8c94e51ddb24c8509 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 9 Apr 2021 22:54:48 -0700 Subject: [PATCH 4/7] Additional testing that uncovered contract violation. ComWrappers API test passes GCStress=0xf --- src/coreclr/src/vm/interoplibinterface.cpp | 8 +++----- .../ReferenceTrackerRuntime.cpp | 12 ++++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/coreclr/src/vm/interoplibinterface.cpp b/src/coreclr/src/vm/interoplibinterface.cpp index 75ec59d35a2a64..933966ccc5c203 100644 --- a/src/coreclr/src/vm/interoplibinterface.cpp +++ b/src/coreclr/src/vm/interoplibinterface.cpp @@ -1029,7 +1029,8 @@ namespace InteropLibImports CONTRACTL { NOTHROW; - MODE_PREEMPTIVE; + GC_NOTRIGGER; + MODE_ANY; PRECONDITION(handle != NULL); } CONTRACTL_END; @@ -1037,14 +1038,11 @@ namespace InteropLibImports bool isValid = false; ::OBJECTHANDLE objectHandle = static_cast<::OBJECTHANDLE>(handle); - HRESULT hr = S_OK; - BEGIN_EXTERNAL_ENTRYPOINT(&hr) { // Switch to cooperative mode so the handle can be safely inspected. - GCX_COOP(); + GCX_COOP_THREAD_EXISTS(GET_THREAD()); isValid = ObjectFromHandle(objectHandle) != NULL; } - END_EXTERNAL_ENTRYPOINT; return isValid; } diff --git a/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp b/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp index fd4ee2906effa8..0cf96a1446c11c 100644 --- a/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp +++ b/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp @@ -203,9 +203,17 @@ namespace { assert(c != nullptr && id != nullptr); + ComSmartPtr mowMaybe; + if (S_OK == c->QueryInterface(&mowMaybe)) + { + (void)mowMaybe->AddRefFromReferenceTracker(); + c = mowMaybe.p; + } + try { *id = _elementId; + if (!_elements.insert(std::make_pair(*id, ComSmartPtr{ c })).second) return S_FALSE; @@ -216,10 +224,6 @@ namespace return E_OUTOFMEMORY; } - ComSmartPtr mowMaybe; - if (S_OK == c->QueryInterface(&mowMaybe)) - (void)mowMaybe->AddRefFromReferenceTracker(); - return S_OK; } From fcc6ed79ee8cb2dda636d58a938c5ba814a8b151 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 13 Apr 2021 18:29:44 -0700 Subject: [PATCH 5/7] Remove noisey asserts. --- src/coreclr/src/interop/comwrappers.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/src/interop/comwrappers.cpp b/src/coreclr/src/interop/comwrappers.cpp index 7307f73aa4fa50..7cadc4e5f59e4b 100644 --- a/src/coreclr/src/interop/comwrappers.cpp +++ b/src/coreclr/src/interop/comwrappers.cpp @@ -694,13 +694,11 @@ HRESULT ManagedObjectWrapper::QueryInterface( ULONG ManagedObjectWrapper::AddRef(void) { - _ASSERTE((_refCount & DestroySentinel) == 0); return GetComCount(::InterlockedIncrement64(&_refCount)); } ULONG ManagedObjectWrapper::Release(void) { - _ASSERTE((_refCount & DestroySentinel) == 0); if (GetComCount(_refCount) == 0) { _ASSERTE(!"Over release of MOW - COM"); From 29965e2acdee2bd246fc7b3b179e0f75620aef9b Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 13 Apr 2021 21:29:50 -0700 Subject: [PATCH 6/7] Remove incorrect assert. --- src/coreclr/src/interop/comwrappers.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/src/interop/comwrappers.cpp b/src/coreclr/src/interop/comwrappers.cpp index 7cadc4e5f59e4b..5c99b49bc371bf 100644 --- a/src/coreclr/src/interop/comwrappers.cpp +++ b/src/coreclr/src/interop/comwrappers.cpp @@ -590,10 +590,7 @@ ULONG ManagedObjectWrapper::ReleaseFromReferenceTracker() // If we observe the destroy sentinel, then this release // must destroy the wrapper. if (refCount == DestroySentinel) - { - _ASSERTE(!IsSet(CreateComInterfaceFlagsEx::IsPegged)); Destroy(this); - } return GetTrackerCount(refCount); } From fd66df21f1cf0e9b6d071c86f60c7a6c20b651bc Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 23 Apr 2021 12:26:31 -0700 Subject: [PATCH 7/7] Add some simple stress logging that recently helped us track down an issue for C#/WinRT. --- src/coreclr/src/vm/interoplibinterface.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/src/vm/interoplibinterface.cpp b/src/coreclr/src/vm/interoplibinterface.cpp index 933966ccc5c203..f9c8912dc1ead9 100644 --- a/src/coreclr/src/vm/interoplibinterface.cpp +++ b/src/coreclr/src/vm/interoplibinterface.cpp @@ -699,6 +699,8 @@ namespace ::ZeroMemory(&gc, sizeof(gc)); GCPROTECT_BEGIN(gc); + STRESS_LOG4(LF_INTEROP, LL_INFO1000, "Get or Create EOC: (Identity: 0x%p) (Flags: %x) (Maybe: 0x%p) (ID: %lld)\n", identity, flags, OBJECTREFToObject(wrapperMaybe), wrapperId); + gc.implRef = impl; gc.wrapperMaybeRef = wrapperMaybe; @@ -731,6 +733,8 @@ namespace } } + STRESS_LOG2(LF_INTEROP, LL_INFO1000, "EOC: 0x%p or Handle: 0x%p\n", extObjCxt, handle); + if (extObjCxt != NULL) { gc.objRefMaybe = extObjCxt->GetObjectRef(); @@ -800,6 +804,8 @@ namespace extObjCxt = cache->FindOrAdd(cacheKey, resultHolder.GetContext()); } + STRESS_LOG2(LF_INTEROP, LL_INFO100, "EOC cache insert: 0x%p == 0x%p\n", extObjCxt, resultHolder.GetContext()); + // If the returned context matches the new context it means the // new context was inserted or a unique instance was requested. if (extObjCxt == resultHolder.GetContext()) @@ -843,6 +849,8 @@ namespace } } + STRESS_LOG3(LF_INTEROP, LL_INFO1000, "EOC: 0x%p, 0x%p => 0x%p\n", extObjCxt, identity, OBJECTREFToObject(gc.objRefMaybe)); + GCPROTECT_END(); *objRef = gc.objRefMaybe;