From 53d2560e67d4be085f5bc22a86debcaed51de2a0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 24 Sep 2021 10:23:55 -0700 Subject: [PATCH] [release/6.0-rc2] Check if External Object Context is still active after a possible GC (#59436) * Check if External Object Context is still active after a possible GC * Update interoplibinterface_comwrappers.cpp Check if `IsActive()` during consideration too. * Issues can occur during the multiple managed allocations. * Add validation of non-null elements in ReleaseObjects() API call. Use detach state instead of IsActive state for EOC. * Apply suggestions from code review Co-authored-by: Elinor Fung * Apply suggestions from code review Co-authored-by: Elinor Fung * Review feedback Co-authored-by: Aaron Robinson Co-authored-by: Elinor Fung --- src/coreclr/vm/interoplibinterface_comwrappers.cpp | 84 +++++++++++++++++----- .../ComWrappers/GlobalInstance/GlobalInstance.cs | 5 ++ 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/src/coreclr/vm/interoplibinterface_comwrappers.cpp b/src/coreclr/vm/interoplibinterface_comwrappers.cpp index 500e8e8..288e706 100644 --- a/src/coreclr/vm/interoplibinterface_comwrappers.cpp +++ b/src/coreclr/vm/interoplibinterface_comwrappers.cpp @@ -312,52 +312,100 @@ namespace GC_TRIGGERS; MODE_COOPERATIVE; PRECONDITION(!IsLockHeld()); + PRECONDITION(!(withFlags & ExternalObjectContext::Flags_Collected)); + PRECONDITION(!(withFlags & ExternalObjectContext::Flags_Detached)); POSTCONDITION(RETVAL != NULL); } CONTRACT_END; struct { + PTRARRAYREF arrRefTmp; PTRARRAYREF arrRef; } gc; ::ZeroMemory(&gc, sizeof(gc)); GCPROTECT_BEGIN(gc); + // Only add objects that are in the correct thread + // context, haven't been detached from the cache, and have + // the appropriate flags set. + // Define a macro predicate since it used in multiple places. + // If an instance is in the hashmap, it is active. This invariant + // holds because the GC is what marks and removes from the cache. +#define SELECT_OBJECT(XX) XX->ThreadContext == threadContext \ + && !XX->IsSet(ExternalObjectContext::Flags_Detached) \ + && (withFlags == ExternalObjectContext::Flags_None || XX->IsSet(withFlags)) + + // Determine the count of objects to return. + SIZE_T objCountMax = 0; + { + LockHolder lock(this); + Iterator end = _hashMap.End(); + for (Iterator curr = _hashMap.Begin(); curr != end; ++curr) + { + ExternalObjectContext* inst = *curr; + if (SELECT_OBJECT(inst)) + { + objCountMax++; + } + } + } + + // Allocate enumerable type to return. + gc.arrRef = (PTRARRAYREF)AllocateObjectArray((DWORD)objCountMax, g_pObjectClass); + CQuickArrayList localList; - // Determine objects to return + // Iterate over the hashmap again while populating the above array + // using the same predicate as before and holding onto context instances. + SIZE_T objCount = 0; + if (0 < objCountMax) { LockHolder lock(this); Iterator end = _hashMap.End(); for (Iterator curr = _hashMap.Begin(); curr != end; ++curr) { ExternalObjectContext* inst = *curr; - - // Only add objects that are in the correct thread - // context and have the appropriate flags set. - if (inst->ThreadContext == threadContext - && (withFlags == ExternalObjectContext::Flags_None || inst->IsSet(withFlags))) + if (SELECT_OBJECT(inst)) { localList.Push(inst); - STRESS_LOG1(LF_INTEROP, LL_INFO100, "Add EOC to Enumerable: 0x%p\n", inst); + + gc.arrRef->SetAt(objCount, inst->GetObjectRef()); + objCount++; + + STRESS_LOG1(LF_INTEROP, LL_INFO1000, "Add EOC to Enumerable: 0x%p\n", inst); } + + // There is a chance more objects were added to the hash while the + // lock was released during array allocation. Once we hit the computed max + // we stop to avoid looking longer than needed. + if (objCount == objCountMax) + break; } } - // Allocate enumerable type to return. - gc.arrRef = (PTRARRAYREF)AllocateObjectArray((DWORD)localList.Size(), g_pObjectClass); - - // Insert objects into enumerable. - // The ExternalObjectContexts in the hashmap are only - // removed and associated objects collected during a GC. Since - // this code is running in Cooperative mode they will never - // be null. - for (SIZE_T i = 0; i < localList.Size(); i++) +#undef SELECT_OBJECT + + // During the allocation of the array to return, a GC could have + // occurred and objects detached from this cache. In order to avoid + // having null array elements we will allocate a new array. + // This subsequent allocation is okay because the array we are + // replacing extends all object lifetimes. + if (objCount < objCountMax) { - ExternalObjectContext* inst = localList[i]; - gc.arrRef->SetAt(i, inst->GetObjectRef()); + gc.arrRefTmp = (PTRARRAYREF)AllocateObjectArray((DWORD)objCount, g_pObjectClass); + + void* dest = gc.arrRefTmp->GetDataPtr(); + void* src = gc.arrRef->GetDataPtr(); + SIZE_T elementSize = gc.arrRef->GetComponentSize(); + + memmoveGCRefs(dest, src, objCount * elementSize); + gc.arrRef = gc.arrRefTmp; } + // All objects are now referenced from the array so won't be collected + // at this point. This means we can safely iterate over the ExternalObjectContext + // instances. { // Separate the wrapper from the tracker runtime prior to // passing them onto the caller. This call is okay to make diff --git a/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs b/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs index 7a358a9..38c2532 100644 --- a/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs +++ b/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs @@ -171,6 +171,11 @@ namespace ComWrappersTests.GlobalInstance protected override void ReleaseObjects(IEnumerable objects) { + foreach (object o in objects) + { + Assert.IsNotNull(o); + } + throw new Exception() { HResult = ReleaseObjectsCallAck }; } -- 2.7.4