Skip to content

Commit

Permalink
Fix ComWrappers' leak in aggregation scenario
Browse files Browse the repository at this point in the history 
Convert Managed Object Wrapper (MOW) GC Handle from HNDTYPE_STRONG
    to HNDTYPE_REFCOUNTED.

Add new CreateObjectFlags value to indicate aggregation during
    CreateObject scenario. This isn't reflected in the managed
    .NET 5 API surface area.

In the ReferenceTracker scenario the ref count may never reach 0 so
    the MOW destructor needs to handle that case. The previous expectation
    for a null in the destructor was based on the STRONG handle logic.

During aggregation scenarios involving ReferenceTracker, ownership
    of the inner is now the responsibility of the runtime.
  • Loading branch information
@AaronRobinsonMSFT
AaronRobinsonMSFT committed Dec 5, 2020
1 parent c2404a9 commit ed9d7c2
Show file tree
Hide file tree
Showing 9 changed files with 359 additions and 150 deletions.
215 changes: 150 additions & 65 deletions src/coreclr/src/interop/comwrappers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,10 @@ namespace
namespace
{
const int32_t TrackerRefShift = 32;
const ULONGLONG TrackerRefCounter = ULONGLONG{ 1 } << TrackerRefShift;
const ULONGLONG ComRefCounter = ULONGLONG{ 1 };
const ULONGLONG TrackerRefZero = 0x0000000080000000;
const ULONGLONG TrackerRefCounter = ULONGLONG{ 1 } << TrackerRefShift;
const ULONGLONG DestroySentinel = 0x0000000080000000;
const ULONGLONG TrackerRefCountMask = 0xffffffff00000000;
const ULONGLONG ComRefCountMask = 0x000000007fffffff;
const ULONGLONG RefCountMask = 0xffffffff7fffffff;

constexpr ULONG GetTrackerCount(_In_ ULONGLONG c)
{
Expand Down Expand Up @@ -419,11 +417,29 @@ HRESULT ManagedObjectWrapper::Create(
void ManagedObjectWrapper::Destroy(_In_ ManagedObjectWrapper* wrapper)
{
_ASSERTE(wrapper != nullptr);
_ASSERTE(GetComCount(wrapper->_refCount) == 0);

// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~ManagedObjectWrapper();
InteropLibImports::MemFree(wrapper, AllocScenario::ManagedObjectWrapper);
// Attempt to set the destroyed bit.
LONGLONG refCount;
LONGLONG prev;
do
{
prev = wrapper->_refCount;
refCount = prev | DestroySentinel;
} while (::InterlockedCompareExchange64(&wrapper->_refCount, refCount, prev) != prev);

// The destroy sentinel represents the bit that indicates the wrapper
// should be destroyed. Since the reference count field (64-bit) holds
// two counters we rely on the singular sentinal value - no other bits
// in the 64-bit counter are set. If there are outstanding bits set it
// indicates there are still outstanding references.
if (refCount == DestroySentinel)
{
// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~ManagedObjectWrapper();
InteropLibImports::MemFree(wrapper, AllocScenario::ManagedObjectWrapper);
}
}

ManagedObjectWrapper::ManagedObjectWrapper(
Expand All @@ -449,48 +465,9 @@ ManagedObjectWrapper::ManagedObjectWrapper(

ManagedObjectWrapper::~ManagedObjectWrapper()
{
// If the target isn't null, then a managed object
// is going to leak.
_ASSERTE(Target == nullptr);
}

ULONGLONG ManagedObjectWrapper::UniversalRelease(_In_ ULONGLONG dec)
{
OBJECTHANDLE local = Target;

LONGLONG refCount;
if (dec == ComRefCounter)
{
_ASSERTE(dec == 1);
refCount = ::InterlockedDecrement64(&_refCount);
}
else
{
_ASSERTE(dec == TrackerRefCounter);
LONGLONG prev;
do
{
prev = _refCount;
refCount = prev - dec;
} while (::InterlockedCompareExchange64(&_refCount, refCount, prev) != prev);
}

// It is possible that a target wasn't set during an
// attempt to reactive the wrapper.
if ((RefCountMask & refCount) == 0 && local != nullptr)
{
_ASSERTE(!IsSet(CreateComInterfaceFlagsEx::IsPegged));
_ASSERTE(refCount == TrackerRefZero || refCount == 0);

// Attempt to reset the target if its current value is the same.
// It is possible the wrapper is in the middle of being reactivated.
(void)TrySetObjectHandle(nullptr, local);

// Tell the runtime to delete the managed object instance handle.
InteropLibImports::DeleteObjectInstanceHandle(local);
}

return refCount;
// If the target isn't null, then release it.
if (Target != nullptr)
InteropLibImports::DeleteObjectInstanceHandle(Target);
}

void* ManagedObjectWrapper::AsRuntimeDefined(_In_ REFIID riid)
Expand Down Expand Up @@ -551,16 +528,18 @@ void ManagedObjectWrapper::ResetFlag(_In_ CreateComInterfaceFlagsEx flag)
::InterlockedAnd((LONG*)&_flags, resetMask);
}

ULONG ManagedObjectWrapper::IsActiveAddRef()
bool ManagedObjectWrapper::IsRooted() const
{
ULONG count = GetComCount(::InterlockedIncrement64(&_refCount));
if (count == 1)
bool rooted = GetComCount(_refCount) > 0;
if (!rooted)
{
// Ensure the current target is null.
::InterlockedExchangePointer(&Target, nullptr);
// Only consider tracker ref count to be a "strong" ref count if it is pegged and alive.
rooted = (GetTrackerCount(_refCount) > 0)
&& (IsSet(CreateComInterfaceFlagsEx::IsPegged)
|| InteropLibImports::GetGlobalPeggingState());
}

return count;
return rooted;
}

ULONG ManagedObjectWrapper::AddRefFromReferenceTracker()
Expand All @@ -578,7 +557,29 @@ ULONG ManagedObjectWrapper::AddRefFromReferenceTracker()

ULONG ManagedObjectWrapper::ReleaseFromReferenceTracker()
{
return GetTrackerCount(UniversalRelease(TrackerRefCounter));
if (GetTrackerCount(_refCount) == 0)
{
_ASSERTE(!"Over release of MOW - ReferenceTracker");
return (ULONG)-1;
}

LONGLONG refCount;
LONGLONG prev;
do
{
prev = _refCount;
refCount = prev - TrackerRefCounter;
} while (::InterlockedCompareExchange64(&_refCount, refCount, prev) != prev);

// 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);
}

HRESULT ManagedObjectWrapper::Peg()
Expand Down Expand Up @@ -652,12 +653,20 @@ HRESULT ManagedObjectWrapper::QueryInterface(

ULONG ManagedObjectWrapper::AddRef(void)
{
_ASSERTE((_refCount & DestroySentinel) == 0);
return GetComCount(::InterlockedIncrement64(&_refCount));
}

ULONG ManagedObjectWrapper::Release(void)
{
return GetComCount(UniversalRelease(ComRefCounter));
_ASSERTE((_refCount & DestroySentinel) == 0);
if (GetComCount(_refCount) == 0)
{
_ASSERTE(!"Over release of MOW - COM");
return (ULONG)-1;
}

return GetComCount(::InterlockedDecrement64(&_refCount));
}

namespace
Expand All @@ -684,12 +693,19 @@ NativeObjectWrapperContext* NativeObjectWrapperContext::MapFromRuntimeContext(_I

HRESULT NativeObjectWrapperContext::Create(
_In_ IUnknown* external,
_In_opt_ IUnknown* inner,
_In_ InteropLib::Com::CreateObjectFlags flags,
_In_ size_t runtimeContextSize,
_Outptr_ NativeObjectWrapperContext** context)
{
_ASSERTE(external != nullptr && context != nullptr);

// Aggregated inners are only currently supported for Aggregated
// scenarios involving IReferenceTracker.
_ASSERTE(inner == nullptr
|| ((flags & InteropLib::Com::CreateObjectFlags_TrackerObject)
&& (flags & InteropLib::Com::CreateObjectFlags_Aggregated)));

HRESULT hr;

ComHolder<IReferenceTracker> trackerObject;
Expand All @@ -710,7 +726,7 @@ HRESULT NativeObjectWrapperContext::Create(
// Contract specifically requires zeroing out runtime context.
::memset(runtimeContext, 0, runtimeContextSize);

NativeObjectWrapperContext* contextLocal = new (cxtMem) NativeObjectWrapperContext{ runtimeContext, trackerObject };
NativeObjectWrapperContext* contextLocal = new (cxtMem) NativeObjectWrapperContext{ runtimeContext, trackerObject, inner };

if (trackerObject != nullptr)
{
Expand All @@ -722,6 +738,13 @@ HRESULT NativeObjectWrapperContext::Create(
Destroy(contextLocal);
return hr;
}

// Aggregation with a tracker object must be "cleaned up".
if (flags & InteropLib::Com::CreateObjectFlags_Aggregated)
{
_ASSERTE(inner != nullptr);
contextLocal->HandleReferenceTrackerAggregation();
}
}

*context = contextLocal;
Expand All @@ -732,28 +755,59 @@ void NativeObjectWrapperContext::Destroy(_In_ NativeObjectWrapperContext* wrappe
{
_ASSERTE(wrapper != nullptr);

// Check if the tracker object manager should be informed prior to being destroyed.
IReferenceTracker* trackerMaybe = wrapper->GetReferenceTracker();
if (trackerMaybe != nullptr)
{
// We only call this during a GC so ignore the failure as
// there is no way we can handle it at this point.
HRESULT hr = TrackerObjectManager::BeforeWrapperDestroyed(trackerMaybe);
_ASSERTE(SUCCEEDED(hr));
(void)hr;
}

// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~NativeObjectWrapperContext();
InteropLibImports::MemFree(wrapper, AllocScenario::NativeObjectWrapper);
}

NativeObjectWrapperContext::NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject)
namespace
{
// State ownership mechanism.
enum : int
{
TrackerObjectState_NotSet = 0,
TrackerObjectState_SetNoRelease = 1,
TrackerObjectState_SetForRelease = 2,
};
}

NativeObjectWrapperContext::NativeObjectWrapperContext(
_In_ void* runtimeContext,
_In_opt_ IReferenceTracker* trackerObject,
_In_opt_ IUnknown* nativeObjectAsInner)
: _trackerObject{ trackerObject }
, _runtimeContext{ runtimeContext }
, _isValidTracker{ (trackerObject != nullptr ? TRUE : FALSE) }
, _trackerObjectDisconnected{ FALSE }
, _trackerObjectState{ (trackerObject == nullptr ? TrackerObjectState_NotSet : TrackerObjectState_SetForRelease) }
, _nativeObjectAsInner{ nativeObjectAsInner }
#ifdef _DEBUG
, _sentinel{ LiveContextSentinel }
#endif
{
if (_isValidTracker == TRUE)
if (_trackerObjectState == TrackerObjectState_SetForRelease)
(void)_trackerObject->AddRef();
}

NativeObjectWrapperContext::~NativeObjectWrapperContext()
{
DisconnectTracker();

// If the inner was supplied, we need to release our reference.
if (_nativeObjectAsInner != nullptr)
(void)_nativeObjectAsInner->Release();

#ifdef _DEBUG
_sentinel = DeadContextSentinel;
#endif
Expand All @@ -766,12 +820,43 @@ void* NativeObjectWrapperContext::GetRuntimeContext() const noexcept

IReferenceTracker* NativeObjectWrapperContext::GetReferenceTracker() const noexcept
{
return ((_isValidTracker == TRUE) ? _trackerObject : nullptr);
return ((_trackerObjectState == TrackerObjectState_NotSet) ? nullptr : _trackerObject);
}

// See TrackerObjectManager::AfterWrapperCreated() for AddRefFromTrackerSource() usage.
// See NativeObjectWrapperContext::HandleReferenceTrackerAggregation() for additional
// cleanup logistics.
void NativeObjectWrapperContext::DisconnectTracker() noexcept
{
// Attempt to disconnect from the tracker.
if (TRUE == ::InterlockedCompareExchange((LONG*)&_isValidTracker, FALSE, TRUE))
// Return if already disconnected or the tracker isn't set.
if (FALSE != ::InterlockedCompareExchange((LONG*)&_trackerObjectDisconnected, TRUE, FALSE)
|| _trackerObjectState == TrackerObjectState_NotSet)
{
return;
}

_ASSERTE(_trackerObject != nullptr);

// Always release the tracker source during a disconnect.
// This to account for the implied IUnknown ownership by the runtime.
(void)_trackerObject->ReleaseFromTrackerSource(); // IUnknown

// Disconnect from the tracker.
if (_trackerObjectState == TrackerObjectState_SetForRelease)
{
(void)_trackerObject->ReleaseFromTrackerSource(); // IReferenceTracker
(void)_trackerObject->Release();
}
}

void NativeObjectWrapperContext::HandleReferenceTrackerAggregation() noexcept
{
_ASSERTE(_trackerObjectState == TrackerObjectState_SetForRelease && _trackerObject != nullptr);

// Aggregation with an IReferenceTracker instance creates an extra AddRef()
// on the outer (e.g. MOW) so we clean up that issue here.
_trackerObjectState = TrackerObjectState_SetNoRelease;

(void)_trackerObject->ReleaseFromTrackerSource(); // IReferenceTracker
(void)_trackerObject->Release();
}
18 changes: 10 additions & 8 deletions src/coreclr/src/interop/comwrappers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ class ManagedObjectWrapper

~ManagedObjectWrapper();

// Represents a single implementation of how to release
// the wrapper. Supplied with a decrementing value.
ULONGLONG UniversalRelease(_In_ ULONGLONG dec);

// Query the runtime defined tables.
void* AsRuntimeDefined(_In_ REFIID riid);

Expand All @@ -102,8 +98,8 @@ class ManagedObjectWrapper
void SetFlag(_In_ CreateComInterfaceFlagsEx flag);
void ResetFlag(_In_ CreateComInterfaceFlagsEx flag);

// Used while validating wrapper is active.
ULONG IsActiveAddRef();
// Indicate if the wrapper should be considered a GC root.
bool IsRooted() const;

public: // IReferenceTrackerTarget
ULONG AddRefFromReferenceTracker();
Expand Down Expand Up @@ -139,7 +135,9 @@ class NativeObjectWrapperContext
{
IReferenceTracker* _trackerObject;
void* _runtimeContext;
Volatile<BOOL> _isValidTracker;
Volatile<BOOL> _trackerObjectDisconnected;
int _trackerObjectState;
IUnknown* _nativeObjectAsInner;

#ifdef _DEBUG
size_t _sentinel;
Expand All @@ -151,6 +149,7 @@ class NativeObjectWrapperContext
// Create a NativeObjectWrapperContext instance
static HRESULT NativeObjectWrapperContext::Create(
_In_ IUnknown* external,
_In_opt_ IUnknown* nativeObjectAsInner,
_In_ InteropLib::Com::CreateObjectFlags flags,
_In_ size_t runtimeContextSize,
_Outptr_ NativeObjectWrapperContext** context);
Expand All @@ -159,7 +158,7 @@ class NativeObjectWrapperContext
static void Destroy(_In_ NativeObjectWrapperContext* wrapper);

private:
NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject);
NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject, _In_opt_ IUnknown* nativeObjectAsInner);
~NativeObjectWrapperContext();

public:
Expand All @@ -171,6 +170,9 @@ class NativeObjectWrapperContext

// Disconnect reference tracker instance.
void DisconnectTracker() noexcept;

private:
void HandleReferenceTrackerAggregation() noexcept;
};

// Manage native object wrappers that support IReferenceTracker.
Expand Down
Loading

0 comments on commit ed9d7c2

Please sign in to comment.