Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/5.0] Fix ComWrappers interaction with the IReferenceTracker interface #45622

Merged
merged 2 commits into from
Dec 8, 2020
Merged

[release/5.0] Fix ComWrappers interaction with the IReferenceTracker interface #45622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3226c62
Fix ComWrappers' leak in aggregation scenario
AaronRobinsonMSFT Nov 11, 2020
2ad35ad
ComWrappers tests
AaronRobinsonMSFT Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 150 additions & 65 deletions src/coreclr/src/interop/comwrappers.cpp
View file
Open in desktop
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,
};
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
}

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
Open in desktop
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