Fix issue 47805 (#52183)

Fix issue https://github.com/dotnet/runtime/issues/47805 - in this case, the BGC_MARKED_BY_FGC bit (0x2) set in the method table leaked out and caused issues for the user program.

In the cases that I've been able to repro, this happened because the bit got set for a short object right in front of a pinned plug, and then saved away by enque_pinned_plug.

Later on, in the case of mark & sweep, we check for the bit and reset it, but later we copy the saved object back by calling recover_saved_pinned_info which calls recover_plug_info to do the actual work. This isn't a problem for the compact case, because we copy the saved object back during compact_plug, turn the bit off, then save it again at the end of compact_plug.

The fix is to turn off the extra bits at the beginning of enque_pinned_plug and save_post_plug_info for the copy that is later restored in mark & sweep (there are actually two copies saved, one for use during compact and one for use during mark & sweep). This builds on an earlier fix by Maoni for a similar problem with another bit.

1 files changed, 63 insertions, 26 deletions

diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp
index c7f2b177f52..7db0faacc26 100644
--- a/src/coreclr/gc/gc.cpp
+++ b/src/coreclr/gc/gc.cpp
@@ -4165,7 +4165,16 @@ size_t gcard_of ( uint8_t*);
 // We only do this when we decide to compact.
 #define BGC_MARKED_BY_FGC (size_t)0x2
 #define MAKE_FREE_OBJ_IN_COMPACT (size_t)0x4
-#endif //DOUBLY_LINKED_FL
+#define ALLOWED_SPECIAL_HEADER_BITS (GC_MARKED|BGC_MARKED_BY_FGC|MAKE_FREE_OBJ_IN_COMPACT)
+#else //DOUBLY_LINKED_FL
+#define ALLOWED_SPECIAL_HEADER_BITS (GC_MARKED)
+#endif //!DOUBLY_LINKED_FL
+
+#ifdef HOST_64BIT
+#define SPECIAL_HEADER_BITS (0x7)
+#else
+#define SPECIAL_HEADER_BITS (0x3)
+#endif
 
 #define slot(i, j) ((uint8_t**)(i))[(j)+1]
 
@@ -4267,11 +4276,7 @@ public:
 
     MethodTable    *GetMethodTable() const
     {
-        return( (MethodTable *) (((size_t) RawGetMethodTable()) & (~(GC_MARKED
-#ifdef DOUBLY_LINKED_FL
-            | BGC_MARKED_BY_FGC | MAKE_FREE_OBJ_IN_COMPACT
-#endif //DOUBLY_LINKED_FL
-            ))));
+        return( (MethodTable *) (((size_t) RawGetMethodTable()) & (~SPECIAL_HEADER_BITS)));
     }
 
     void SetMarked()
@@ -4339,6 +4344,26 @@ public:
     }
 #endif //DOUBLY_LINKED_FL
 
+    size_t ClearSpecialBits()
+    {
+        size_t special_bits = ((size_t)RawGetMethodTable()) & SPECIAL_HEADER_BITS;
+        if (special_bits != 0)
+        {
+            assert ((special_bits & (~ALLOWED_SPECIAL_HEADER_BITS)) == 0);
+            RawSetMethodTable ((MethodTable*)(((size_t)RawGetMethodTable()) & ~(SPECIAL_HEADER_BITS)));
+        }
+        return special_bits;
+    }
+
+    void SetSpecialBits (size_t special_bits)
+    {
+        assert ((special_bits & (~ALLOWED_SPECIAL_HEADER_BITS)) == 0);
+        if (special_bits != 0)
+        {
+            RawSetMethodTable ((MethodTable*)(((size_t)RawGetMethodTable()) | special_bits));
+        }
+    }
+
     CGCDesc *GetSlotMap ()
     {
         assert (GetMethodTable()->ContainsPointers());
@@ -4506,6 +4531,17 @@ inline
 BOOL is_plug_padded (uint8_t* node){return FALSE;}
 #endif //SHORT_PLUGS
 
+inline
+size_t clear_special_bits (uint8_t* node)
+{
+    return header(node)->ClearSpecialBits();
+}
+
+inline
+void set_special_bits (uint8_t* node, size_t special_bits)
+{
+    header(node)->SetSpecialBits (special_bits);
+}
 
 inline size_t unused_array_size(uint8_t * p)
 {
@@ -20978,16 +21014,16 @@ void gc_heap::enque_pinned_plug (uint8_t* plug,
 
     if (save_pre_plug_info_p)
     {
-#ifdef SHORT_PLUGS
-        BOOL is_padded = is_plug_padded (last_object_in_last_plug);
-        if (is_padded)
-            clear_plug_padded (last_object_in_last_plug);
-#endif //SHORT_PLUGS
+        // In the case of short plugs or doubly linked free lists, there may be extra bits
+        // set in the method table pointer.
+        // Clear these bits for the copy saved in saved_pre_plug, but not for the copy
+        // saved in saved_pre_plug_reloc.
+        // This is because we need these bits for compaction, but not for mark & sweep.
+        size_t special_bits = clear_special_bits (last_object_in_last_plug);
+        // now copy the bits over
         memcpy (&(m.saved_pre_plug), &(((plug_and_gap*)plug)[-1]), sizeof (gap_reloc_pair));
-#ifdef SHORT_PLUGS
-        if (is_padded)
-            set_plug_padded (last_object_in_last_plug);
-#endif //SHORT_PLUGS
+        // restore the bits in the original
+        set_special_bits (last_object_in_last_plug, special_bits);
 
         memcpy (&(m.saved_pre_plug_reloc), &(((plug_and_gap*)plug)[-1]), sizeof (gap_reloc_pair));
 
@@ -20997,7 +21033,7 @@ void gc_heap::enque_pinned_plug (uint8_t* plug,
         {
             record_interesting_data_point (idp_pre_short);
 #ifdef SHORT_PLUGS
-            if (is_padded)
+            if (is_plug_padded (last_object_in_last_plug))
                 record_interesting_data_point (idp_pre_short_padded);
 #endif //SHORT_PLUGS
             dprintf (3, ("encountered a short object %Ix right before pinned plug %Ix!",
@@ -21041,16 +21077,17 @@ void gc_heap::save_post_plug_info (uint8_t* last_pinned_plug, uint8_t* last_obje
     assert (last_pinned_plug == m.first);
     m.saved_post_plug_info_start = (uint8_t*)&(((plug_and_gap*)post_plug)[-1]);
 
-#ifdef SHORT_PLUGS
-    BOOL is_padded = is_plug_padded (last_object_in_last_plug);
-    if (is_padded)
-        clear_plug_padded (last_object_in_last_plug);
-#endif //SHORT_PLUGS
+    // In the case of short plugs or doubly linked free lists, there may be extra bits
+    // set in the method table pointer.
+    // Clear these bits for the copy saved in saved_post_plug, but not for the copy
+    // saved in saved_post_plug_reloc.
+    // This is because we need these bits for compaction, but not for mark & sweep.
+    // Note that currently none of these bits will ever be set in the object saved *after*
+    // a pinned plug - this object is currently pinned along with the pinned object before it
+    size_t special_bits = clear_special_bits (last_object_in_last_plug);
     memcpy (&(m.saved_post_plug), m.saved_post_plug_info_start, sizeof (gap_reloc_pair));
-#ifdef SHORT_PLUGS
-    if (is_padded)
-        set_plug_padded (last_object_in_last_plug);
-#endif //SHORT_PLUGS
+    // restore the bits in the original
+    set_special_bits (last_object_in_last_plug, special_bits);
 
     memcpy (&(m.saved_post_plug_reloc), m.saved_post_plug_info_start, sizeof (gap_reloc_pair));
 
@@ -21069,7 +21106,7 @@ void gc_heap::save_post_plug_info (uint8_t* last_pinned_plug, uint8_t* last_obje
         dprintf (3, ("PP %Ix last obj %Ix is too short", last_pinned_plug, last_object_in_last_plug));
         record_interesting_data_point (idp_post_short);
 #ifdef SHORT_PLUGS
-        if (is_padded)
+        if (is_plug_padded (last_object_in_last_plug))
             record_interesting_data_point (idp_post_short_padded);
 #endif //SHORT_PLUGS
         m.set_post_short();