[release/6.0-rc2] Check if External Object Context is still active after a possible...

* 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 <elfung@microsoft.com>
* Apply suggestions from code review

Co-authored-by: Elinor Fung <elfung@microsoft.com>
* Review feedback

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>

diff --git a/src/coreclr/vm/interoplibinterface_comwrappers.cpp b/src/coreclr/vm/interoplibinterface_comwrappers.cpp
index 500e8e8..288e706 100644 (file)
--- 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<ExternalObjectContext*> 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 (file)
--- 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 };
             }