Fix generic_ivar_set_shape_field for table rebuild

[Bug #21438]

Previously GC could trigger a table rebuild of the generic fields
st_table in the middle of calling the st_update callback. This could
cause entries to be reallocated or rearranged and the update to be for
the wrong entry.

This commit adds an assertion to make that case easier to detect, and
replaces the st_update with a separate st_lookup and st_insert.

Co-authored-by: Aaron Patterson <[email protected]>
Co-authored-by: Jean Boussier <[email protected]>

Author: 3 people

st.c

@@ -1495,7 +1495,16 @@ st_update(st_table *tab, st_data_t key,
         value = entry->record;
     }
     old_key = key;
+
+    unsigned int rebuilds_num = tab->rebuilds_num;
+
     retval = (*func)(&key, &value, arg, existing);
+
+    // We need to make sure that the callback didn't cause a table rebuild
+    // Ideally we would make sure no operations happened
+    assert(rebuilds_num == tab->rebuilds_num);
+    (void)rebuilds_num;
+
     switch (retval) {
       case ST_CONTINUE:
         if (! existing) {

test/ruby/test_variable.rb

@@ -407,6 +407,19 @@ def test_external_ivars
     }
   end
 
+  def test_exivar_resize_with_compaction_stress
+    objs = 10_000.times.map do
+      ExIvar.new
+    end
+    EnvUtil.under_gc_compact_stress do
+      10.times do
+        x = ExIvar.new
+        x.instance_variable_set(:@resize, 1)
+        x
+      end
+    end
+  end
+
   def test_local_variables_with_kwarg
     bug11674 = '[ruby-core:71437] [Bug #11674]'
     v = with_kwargs_11(v1:1,v2:2,v3:3,v4:4,v5:5,v6:6,v7:7,v8:8,v9:9,v10:10,v11:11)

variable.c

@@ -1823,43 +1823,34 @@ struct gen_fields_lookup_ensure_size {
     bool resize;
 };
 
-static int
-generic_fields_lookup_ensure_size(st_data_t *k, st_data_t *v, st_data_t u, int existing)
-{
-    ASSERT_vm_locking();
-
-    struct gen_fields_lookup_ensure_size *fields_lookup = (struct gen_fields_lookup_ensure_size *)u;
-    struct gen_fields_tbl *fields_tbl = existing ? (struct gen_fields_tbl *)*v : NULL;
-
-    if (!existing || fields_lookup->resize) {
-        if (existing) {
-            RUBY_ASSERT(RSHAPE_TYPE_P(fields_lookup->shape_id, SHAPE_IVAR) || RSHAPE_TYPE_P(fields_lookup->shape_id, SHAPE_OBJ_ID));
-            RUBY_ASSERT(RSHAPE_CAPACITY(RSHAPE_PARENT(fields_lookup->shape_id)) < RSHAPE_CAPACITY(fields_lookup->shape_id));
-        }
-
-        fields_tbl = gen_fields_tbl_resize(fields_tbl, RSHAPE_CAPACITY(fields_lookup->shape_id));
-        *v = (st_data_t)fields_tbl;
-    }
-
-    fields_lookup->fields_tbl = fields_tbl;
-    if (fields_lookup->shape_id) {
-        rb_obj_set_shape_id(fields_lookup->obj, fields_lookup->shape_id);
-    }
-
-    RUBY_ASSERT(rb_obj_exivar_p((VALUE)*k));
-
-    return ST_CONTINUE;
-}
-
 static VALUE *
 generic_ivar_set_shape_fields(VALUE obj, void *data)
 {
     RUBY_ASSERT(!rb_shape_obj_too_complex_p(obj));
 
     struct gen_fields_lookup_ensure_size *fields_lookup = data;
 
+    // We can't use st_update, since when resizing the fields table GC can
+    // happen, which will modify the st_table and may rebuild it
     RB_VM_LOCKING() {
-        st_update(generic_fields_tbl(obj, fields_lookup->id, false), (st_data_t)obj, generic_fields_lookup_ensure_size, (st_data_t)fields_lookup);
+        struct gen_fields_tbl *fields_tbl = NULL;
+        st_table *tbl = generic_fields_tbl(obj, fields_lookup->id, false);
+        int existing = st_lookup(tbl, (st_data_t)obj, (st_data_t *)&fields_tbl);
+
+        if (!existing || fields_lookup->resize) {
+            if (existing) {
+                RUBY_ASSERT(RSHAPE_TYPE_P(fields_lookup->shape_id, SHAPE_IVAR) || RSHAPE_TYPE_P(fields_lookup->shape_id, SHAPE_OBJ_ID));
+                RUBY_ASSERT(RSHAPE_CAPACITY(RSHAPE_PARENT(fields_lookup->shape_id)) < RSHAPE_CAPACITY(fields_lookup->shape_id));
+            }
+
+            fields_tbl = gen_fields_tbl_resize(fields_tbl, RSHAPE_CAPACITY(fields_lookup->shape_id));
+            st_insert(tbl, (st_data_t)obj, (st_data_t)fields_tbl);
+        }
+
+        fields_lookup->fields_tbl = fields_tbl;
+        if (fields_lookup->shape_id) {
+            rb_obj_set_shape_id(fields_lookup->obj, fields_lookup->shape_id);
+        }
     }
 
     return fields_lookup->fields_tbl->as.shape.fields;