From 45ddafb95bd732f7305925a779cd8403ac30e1bf Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 18 Jun 2025 18:47:59 +0100 Subject: [PATCH] Fix use-after-free when resizing exivars (#13637) Fix generic_ivar_set_shape_ivptr for table rebuild [Bug #21438] Previously GC could trigger a table rebuild of the generic ivar 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. Also free after insert in generic_ivar_set_shape_ivptr Previously we were performing a realloc and then inserting the new value into the table. If the table was flagged as requiring a rebuild, this could trigger GC work and marking within that GC could access the ivptr freed by realloc. Co-authored-by: John Hawthorn Co-authored-by: Aaron Patterson --- st.c | 9 ++++ test/ruby/test_variable.rb | 22 ++++++++- variable.c | 96 +++++++++++++++++--------------------- 3 files changed, 71 insertions(+), 56 deletions(-) diff --git a/st.c b/st.c index 97ec69121a..6a69022bc9 100644 --- a/st.c +++ b/st.c @@ -1494,7 +1494,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) { diff --git a/test/ruby/test_variable.rb b/test/ruby/test_variable.rb index 49fec2d40e..e8bf0e7ef8 100644 --- a/test/ruby/test_variable.rb +++ b/test/ruby/test_variable.rb @@ -393,20 +393,38 @@ class TestVariable < Test::Unit::TestCase @a = 1 @b = 2 @c = 3 + @d = 4 + @e = 5 + @f = 6 + @g = 7 + @h = 8 end def ivars - [@a, @b, @c] + [@a, @b, @c, @d, @e, @f, @g, @h] end end def test_external_ivars 3.times{ # check inline cache for external ivar access - assert_equal [1, 2, 3], ExIvar.new.ivars + assert_equal [1, 2, 3, 4, 5, 6, 7, 8], ExIvar.new.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) diff --git a/variable.c b/variable.c index 5bd5808a63..82af17002c 100644 --- a/variable.c +++ b/variable.c @@ -1183,22 +1183,6 @@ gen_ivtbl_bytes(size_t n) return offsetof(struct gen_ivtbl, as.shape.ivptr) + n * sizeof(VALUE); } -static struct gen_ivtbl * -gen_ivtbl_resize(struct gen_ivtbl *old, uint32_t n) -{ - RUBY_ASSERT(n > 0); - - uint32_t len = old ? old->as.shape.numiv : 0; - struct gen_ivtbl *ivtbl = xrealloc(old, gen_ivtbl_bytes(n)); - - ivtbl->as.shape.numiv = n; - for (; len < n; len++) { - ivtbl->as.shape.ivptr[len] = Qundef; - } - - return ivtbl; -} - void rb_mark_generic_ivar(VALUE obj) { @@ -1651,41 +1635,6 @@ struct gen_ivar_lookup_ensure_size { bool resize; }; -static int -generic_ivar_lookup_ensure_size(st_data_t *k, st_data_t *v, st_data_t u, int existing) -{ - ASSERT_vm_locking(); - - struct gen_ivar_lookup_ensure_size *ivar_lookup = (struct gen_ivar_lookup_ensure_size *)u; - struct gen_ivtbl *ivtbl = existing ? (struct gen_ivtbl *)*v : NULL; - - if (!existing || ivar_lookup->resize) { - if (existing) { - RUBY_ASSERT(ivar_lookup->shape->type == SHAPE_IVAR); - RUBY_ASSERT(rb_shape_get_shape_by_id(ivar_lookup->shape->parent_id)->capacity < ivar_lookup->shape->capacity); - } - else { - FL_SET_RAW((VALUE)*k, FL_EXIVAR); - } - - ivtbl = gen_ivtbl_resize(ivtbl, ivar_lookup->shape->capacity); - *v = (st_data_t)ivtbl; - } - - RUBY_ASSERT(FL_TEST((VALUE)*k, FL_EXIVAR)); - - ivar_lookup->ivtbl = ivtbl; - if (ivar_lookup->shape) { -#if SHAPE_IN_BASIC_FLAGS - rb_shape_set_shape(ivar_lookup->obj, ivar_lookup->shape); -#else - ivtbl->shape_id = rb_shape_id(ivar_lookup->shape); -#endif - } - - return ST_CONTINUE; -} - static VALUE * generic_ivar_set_shape_ivptr(VALUE obj, void *data) { @@ -1693,9 +1642,48 @@ generic_ivar_set_shape_ivptr(VALUE obj, void *data) struct gen_ivar_lookup_ensure_size *ivar_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_LOCK_ENTER(); { - st_update(generic_ivtbl(obj, ivar_lookup->id, false), (st_data_t)obj, generic_ivar_lookup_ensure_size, (st_data_t)ivar_lookup); + struct gen_ivtbl *ivtbl = NULL; + st_table *tbl = generic_ivtbl(obj, ivar_lookup->id, false); + int existing = st_lookup(tbl, (st_data_t)obj, (st_data_t *)&ivtbl); + + if (!existing || ivar_lookup->resize) { + uint32_t new_capa = ivar_lookup->shape->capacity; + uint32_t old_capa = rb_shape_get_shape_by_id(ivar_lookup->shape->parent_id)->capacity; + + if (existing) { + RUBY_ASSERT(ivar_lookup->shape->type == SHAPE_IVAR); + RUBY_ASSERT(old_capa < new_capa); + RUBY_ASSERT(ivtbl); + } else { + RUBY_ASSERT(!ivtbl); + RUBY_ASSERT(old_capa == 0); + } + RUBY_ASSERT(new_capa > 0); + + struct gen_ivtbl *old_ivtbl = ivtbl; + ivtbl = xmalloc(gen_ivtbl_bytes(new_capa)); + if (old_ivtbl) { + memcpy(ivtbl, old_ivtbl, gen_ivtbl_bytes(old_capa)); + } + ivtbl->as.shape.numiv = new_capa; + for (uint32_t i = old_capa; i < new_capa; i++) { + ivtbl->as.shape.ivptr[i] = Qundef; + } + + st_insert(tbl, (st_data_t)obj, (st_data_t)ivtbl); + if (old_ivtbl) { + xfree(old_ivtbl); + } + } + + ivar_lookup->ivtbl = ivtbl; + if (ivar_lookup->shape) { + rb_shape_set_shape(ivar_lookup->obj, ivar_lookup->shape); + } } RB_VM_LOCK_LEAVE(); @@ -2150,8 +2138,8 @@ rb_copy_generic_ivar(VALUE clone, VALUE obj) new_ivtbl->as.complex.table = st_copy(obj_ivtbl->as.complex.table); } else { - new_ivtbl = gen_ivtbl_resize(0, obj_ivtbl->as.shape.numiv); - + new_ivtbl = xmalloc(gen_ivtbl_bytes(obj_ivtbl->as.shape.numiv)); + new_ivtbl->as.shape.numiv = obj_ivtbl->as.shape.numiv; for (uint32_t i=0; ias.shape.numiv; i++) { RB_OBJ_WRITE(clone, &new_ivtbl->as.shape.ivptr[i], obj_ivtbl->as.shape.ivptr[i]); }