Fix issue 41928 (#48295)

Details:

The issue was that verify_heap called copy_brick_card_table while another thread was growing these tables, so we ended up with a mark_array where the sections for a new segment were not committed, leading to an AV in bgc_verify_mark_array_cleared.

The fix is to take the gc_lock *before* calling veriy_heap. To avoid a deadlock, the lock has to be taken before we suspend the EE. This only affects the end of a background GC, the verify_heap at the start of a background GC already happens under the gc_lock.

I factored out common code into new methods enter_gc_lock_for_verify_heap and leave_gc_lock_for_verify_heap.

diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp
index 6bbe250..73dd3f6 100644 (file)
--- a/src/coreclr/gc/gc.cpp
+++ b/src/coreclr/gc/gc.cpp
@@ -18999,6 +18999,10 @@ void gc_heap::gc1()
             }
             if (heap_number == 0)
             {
+                // need to take the gc_lock in preparation for verify_heap below
+                // *before* we suspend the EE, otherwise we get a deadlock
+                enter_gc_lock_for_verify_heap();
+
                 suspend_EE();
                 bgc_threads_sync_event.Set();
             }
@@ -19008,6 +19012,10 @@ void gc_heap::gc1()
                 dprintf (2, ("bgc_threads_sync_event is signalled"));
             }
 #else //MULTIPLE_HEAPS
+            // need to take the gc_lock in preparation for verify_heap below
+            // *before* we suspend the EE, otherwise we get a deadlock
+            enter_gc_lock_for_verify_heap();
+
             suspend_EE();
 #endif //MULTIPLE_HEAPS
 
@@ -19056,6 +19064,7 @@ void gc_heap::gc1()
             if (heap_number == 0)
             {
                 restart_EE();
+                leave_gc_lock_for_verify_heap();
                 bgc_threads_sync_event.Set();
             }
             else
@@ -19064,7 +19073,9 @@ void gc_heap::gc1()
                 dprintf (2, ("bgc_threads_sync_event is signalled"));
             }
 #else //MULTIPLE_HEAPS
+
             restart_EE();
+            leave_gc_lock_for_verify_heap();
 #endif //MULTIPLE_HEAPS
 
             disable_preemptive (cooperative_mode);
@@ -39211,6 +39222,28 @@ BOOL gc_heap::check_need_card (uint8_t* child_obj, int gen_num_for_cards,
 #endif //USE_REGIONS
 }
 
+void gc_heap::enter_gc_lock_for_verify_heap()
+{
+#ifdef VERIFY_HEAP
+    if (GCConfig::GetHeapVerifyLevel() & GCConfig::HEAPVERIFY_GC)
+    {
+        enter_spin_lock (&gc_heap::gc_lock);
+        dprintf (SPINLOCK_LOG, ("enter gc_lock for verify_heap"));
+    }
+#endif // VERIFY_HEAP
+}
+
+void gc_heap::leave_gc_lock_for_verify_heap()
+{
+#ifdef VERIFY_HEAP
+    if (GCConfig::GetHeapVerifyLevel() & GCConfig::HEAPVERIFY_GC)
+    {
+        dprintf (SPINLOCK_LOG, ("leave gc_lock taken for verify_heap"));
+        leave_spin_lock (&gc_heap::gc_lock);
+    }
+#endif // VERIFY_HEAP
+}
+
 void gc_heap::verify_heap (BOOL begin_gc_p)
 {
     int heap_verify_level = static_cast<int>(GCConfig::GetHeapVerifyLevel());

diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h
index d5d5fcd..53b3c1b 100644 (file)
--- a/src/coreclr/gc/gcpriv.h
+++ b/src/coreclr/gc/gcpriv.h
@@ -1217,6 +1217,10 @@ public:
     void verify_regions (int gen_number);
     PER_HEAP
     void verify_regions();
+    PER_HEAP_ISOLATED
+    void enter_gc_lock_for_verify_heap();
+    PER_HEAP_ISOLATED
+    void leave_gc_lock_for_verify_heap();
     PER_HEAP
     void verify_heap (BOOL begin_gc_p);
     PER_HEAP