Fix recently introduced timing issue when growing the card table

Observed issue:
- When software write watch is enabled, the runtime is suspended in grow_brick_card_tables() to copy the dirty state of pages into the new table
- The global card table pointer was updated before the attempt to suspend the runtime
- That thread gets blocked there, while the background GC thread successfully suspends the runtime, and calls copy_brick_card_table()
- Since the lowest and highest addresses of the GC heap were not updated, copy_brick_card_table() reads invalid memory

Fixes:
- Moved the change to g_card_table to after the point of suspend for software write watch. At the point of this suspend, the global state will be that prior to resize.
- Though much less frequent, this issue can also occur with the implicit suspend that occurs as part of StompWriteBarrierResize(), when the write barrier type needs to be changed. Moved the other StompWriteBarrierResize() down such that it is done after all global state is updated. At the point of this suspend, the global state will be that following resize.
- I don't think we can have a failure path (failing to commit the mark array) after updating global state such as g_card_table. Soon after that, there may be changes to the new table on paths that don't go through the write barrier, and upon failure to commit the mark array, those updates would be lost. Passed the new card table to functions that need them, and moved changes to global table pointers to after all of the failure points.

Fixes #4750

2 files changed, 64 insertions, 46 deletions

diff --git a/src/gc/gc.cpp b/src/gc/gc.cpp
index 0ad36aa2db..5125eb0ebc 100644
--- a/src/gc/gc.cpp
+++ b/src/gc/gc.cpp
@@ -7067,9 +7067,11 @@ int gc_heap::grow_brick_card_tables (uint8_t* start,
                                 (size_t)saved_g_lowest_address,
                                 (size_t)saved_g_highest_address));
 
+        bool write_barrier_updated = false;
         uint32_t virtual_reserve_flags = VirtualReserveFlags::None;
         uint32_t* saved_g_card_table = g_card_table;
         uint32_t* ct = 0;
+        uint32_t* translated_ct = 0;
         short* bt = 0;
 
         size_t cs = size_card_of (saved_g_lowest_address, saved_g_highest_address);
@@ -7189,10 +7191,10 @@ int gc_heap::grow_brick_card_tables (uint8_t* start,
             card_table_mark_array (ct) = NULL;
 #endif //MARK_ARRAY
 
-        g_card_table = translate_card_table (ct);
+        translated_ct = translate_card_table (ct);
 
         dprintf (GC_TABLE_LOG, ("card table: %Ix(translated: %Ix), seg map: %Ix, mark array: %Ix", 
-            (size_t)ct, (size_t)g_card_table, (size_t)seg_mapping_table, (size_t)card_table_mark_array (ct)));
+            (size_t)ct, (size_t)translated_ct, (size_t)seg_mapping_table, (size_t)card_table_mark_array (ct)));
 
 #ifdef BACKGROUND_GC
         if (hp->should_commit_mark_array())
@@ -7209,7 +7211,7 @@ int gc_heap::grow_brick_card_tables (uint8_t* start,
                 goto fail;
             }
 
-            if (!commit_mark_array_new_seg (hp, new_seg, saved_g_lowest_address))
+            if (!commit_mark_array_new_seg (hp, new_seg, translated_ct, saved_g_lowest_address))
             {
                 dprintf (GC_TABLE_LOG, ("failed to commit mark array for the new seg"));
                 set_fgm_result (fgm_commit_table, logging_ma_commit_size, loh_p);
@@ -7222,51 +7224,50 @@ int gc_heap::grow_brick_card_tables (uint8_t* start,
         }
 #endif //BACKGROUND_GC
 
-        {
-            bool write_barrier_updated = false;
 #ifdef FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP
-            if (gc_can_use_concurrent)
-            {
-                // The current design of software write watch requires that the runtime is suspended during resize. Suspending
-                // on resize is preferred because it is a far less frequent operation than GetWriteWatch() / ResetWriteWatch().
-                // Suspending here allows copying dirty state from the old table into the new table, and not have to merge old
-                // table info lazily as done for card tables.
+        if (gc_can_use_concurrent)
+        {
+            // The current design of software write watch requires that the runtime is suspended during resize. Suspending
+            // on resize is preferred because it is a far less frequent operation than GetWriteWatch() / ResetWriteWatch().
+            // Suspending here allows copying dirty state from the old table into the new table, and not have to merge old
+            // table info lazily as done for card tables.
 
-                BOOL is_runtime_suspended = IsSuspendEEThread();
-                if (!is_runtime_suspended)
-                {
-                    suspend_EE();
-                }
+            BOOL is_runtime_suspended = IsSuspendEEThread();
+            if (!is_runtime_suspended)
+            {
+                // Note on points where the runtime is suspended anywhere in this function. Upon an attempt to suspend the
+                // runtime, a different thread may suspend first, causing this thread to block at the point of the suspend call.
+                // So, at any suspend point, externally visible state needs to be consistent, as code that depends on that state
+                // may run while this thread is blocked. This includes updates to g_card_table, g_lowest_address, and
+                // g_highest_address.
+                suspend_EE();
+            }
 
-                SoftwareWriteWatch::SetResizedUntranslatedTable(
-                    mem + sw_ww_table_offset,
-                    saved_g_lowest_address,
-                    saved_g_highest_address);
+            g_card_table = translated_ct;
 
-                // Since the runtime is already suspended, update the write barrier here as well.
-                // This passes a bool telling whether we need to switch to the post
-                // grow version of the write barrier.  This test tells us if the new
-                // segment was allocated at a lower address than the old, requiring
-                // that we start doing an upper bounds check in the write barrier.
-                StompWriteBarrierResize(true, la != saved_g_lowest_address);
-                write_barrier_updated = true;
+            SoftwareWriteWatch::SetResizedUntranslatedTable(
+                mem + sw_ww_table_offset,
+                saved_g_lowest_address,
+                saved_g_highest_address);
 
-                if (!is_runtime_suspended)
-                {
-                    restart_EE();
-                }
-            }
-#endif // FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP
+            // Since the runtime is already suspended, update the write barrier here as well.
+            // This passes a bool telling whether we need to switch to the post
+            // grow version of the write barrier.  This test tells us if the new
+            // segment was allocated at a lower address than the old, requiring
+            // that we start doing an upper bounds check in the write barrier.
+            StompWriteBarrierResize(true, la != saved_g_lowest_address);
+            write_barrier_updated = true;
 
-            if (!write_barrier_updated)
+            if (!is_runtime_suspended)
             {
-                // This passes a bool telling whether we need to switch to the post
-                // grow version of the write barrier.  This test tells us if the new
-                // segment was allocated at a lower address than the old, requiring
-                // that we start doing an upper bounds check in the write barrier.
-                StompWriteBarrierResize(!!IsSuspendEEThread(), la != saved_g_lowest_address);
+                restart_EE();
             }
         }
+        else
+#endif // FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP
+        {
+            g_card_table = translated_ct;
+        }
 
         // We need to make sure that other threads executing checked write barriers
         // will see the g_card_table update before g_lowest/highest_address updates.
@@ -7279,6 +7280,19 @@ int gc_heap::grow_brick_card_tables (uint8_t* start,
         g_lowest_address = saved_g_lowest_address;
         VolatileStore(&g_highest_address, saved_g_highest_address);
 
+        if (!write_barrier_updated)
+        {
+            // This passes a bool telling whether we need to switch to the post
+            // grow version of the write barrier.  This test tells us if the new
+            // segment was allocated at a lower address than the old, requiring
+            // that we start doing an upper bounds check in the write barrier.
+            // This will also suspend the runtime if the write barrier type needs
+            // to be changed, so we are doing this after all global state has
+            // been updated. See the comment above suspend_EE() above for more
+            // info.
+            StompWriteBarrierResize(!!IsSuspendEEThread(), la != saved_g_lowest_address);
+        }
+
         return 0;
         
 fail:
@@ -7286,10 +7300,7 @@ fail:
 
         if (mem)
         {
-            if (g_card_table != saved_g_card_table)
-            {
-                g_card_table = saved_g_card_table; 
-            }
+            assert(g_card_table == saved_g_card_table);
 
             //delete (uint32_t*)((uint8_t*)ct - sizeof(card_table_info));
             if (!GCToOSInterface::VirtualRelease (mem, alloc_size_aligned))
@@ -25269,6 +25280,7 @@ void gc_heap::verify_mark_array_cleared (heap_segment* seg, uint32_t* mark_array
 
 BOOL gc_heap::commit_mark_array_new_seg (gc_heap* hp, 
                                          heap_segment* seg,
+                                         uint32_t* new_card_table,
                                          uint8_t* new_lowest_address)
 {
     UNREFERENCED_PARAMETER(hp); // compiler bug? -- this *is*, indeed, referenced
@@ -25307,18 +25319,23 @@ BOOL gc_heap::commit_mark_array_new_seg (gc_heap* hp,
             return FALSE;
         }
 
-        if (hp->card_table != g_card_table)
+        if (new_card_table == 0)
+        {
+            new_card_table = g_card_table;
+        }
+
+        if (hp->card_table != new_card_table)
         {
             if (new_lowest_address == 0)
             {
                 new_lowest_address = g_lowest_address;
             }
 
-            uint32_t* ct = &g_card_table[card_word (gcard_of (new_lowest_address))];
+            uint32_t* ct = &new_card_table[card_word (gcard_of (new_lowest_address))];
             uint32_t* ma = (uint32_t*)((uint8_t*)card_table_mark_array (ct) - size_mark_array_of (0, new_lowest_address));
 
             dprintf (GC_TABLE_LOG, ("table realloc-ed: %Ix->%Ix, MA: %Ix->%Ix", 
-                                    hp->card_table, g_card_table,
+                                    hp->card_table, new_card_table,
                                     hp->mark_array, ma));
 
             if (!commit_mark_array_by_range (commit_start, commit_end, ma))
diff --git a/src/gc/gcpriv.h b/src/gc/gcpriv.h
index 64f37c5d35..e14cab4c62 100644
--- a/src/gc/gcpriv.h
+++ b/src/gc/gcpriv.h
@@ -2652,6 +2652,7 @@ protected:
     PER_HEAP_ISOLATED
     BOOL commit_mark_array_new_seg (gc_heap* hp, 
                                     heap_segment* seg,
+                                    uint32_t* new_card_table = 0,
                                     uint8_t* new_lowest_address = 0);
 
     PER_HEAP_ISOLATED