10000 Fix generic_ivar_set_shape_ivptr for table rebuild · ruby/ruby@5461885 · GitHub
[go: up one dir, main page]
Skip to content

Commit 5461885

Browse files
jhawthorntenderlovebyroot
committed
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: Aaron Patterson <tenderlove@ruby-lang.org> Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
1 parent 2cce628 commit 5461885

File tree

3 files changed

+71
-56
lines changed

3 files changed

+71
-56
lines changed

‎st.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1494,7 +1494,16 @@ st_update(st_table *tab, st_data_t key,
14941494
value = entry->record;
14951495
}
14961496
old_key = key;
1497+
1498+
unsigned int rebuilds_num = tab->rebuilds_num;
1499+
14971500
retval = (*func)(&key, &value, arg, existing);
1501+
1502+
// We need to make sure that the callback didn't cause a table rebuild
1503+
// Ideally we would make sure no operations happened
1504+
assert(rebuilds_num == tab->rebuilds_num);
1505+
(void)rebuilds_num;
1506+
14981507
switch (retval) {
14991508
case ST_CONTINUE:
15001509
if (! existing) {

‎test/ruby/test_variable.rb

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,20 +393,38 @@ def initialize
393393
@a = 1
394394
@b = 2
395395
@c = 3
396+
@d = 4
397+
@e = 5
398+
@f = 6
399+
@g = 7
400+
@h = 8
396401
end
397402

398403
def ivars
399-
[@a, @b, @c]
404+
[@a, @b, @c, @d, @e, @f, @g, @h]
400405
end
401406
end
402407

403408
def test_external_ivars
404409
3.times{
405410
# check inline cache for external ivar access
406-
assert_equal [1, 2, 3], ExIvar.new.ivars
411+
assert_equal [1, 2, 3, 4, 5, 6, 7, 8], ExIvar.new.ivars
407412
}
408413
end
409414

415+
def test_exivar_resize_with_compaction_stress
416+
objs = 10_000.times.map do
417+
ExIvar.new
418+
end
419+
EnvUtil.under_gc_compact_stress do
420+
10.times do
421+
x = ExIvar.new
422+
x.instance_variable_set(:@resize, 1)
423+
x
424+
end
425+
end
426+
end
427+
410428
def test_local_variables_with_kwarg
411429
bug11674 = '[ruby-core:71437] [Bug #11674]'
412430
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

Lines changed: 42 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,22 +1183,6 @@ gen_ivtbl_bytes(size_t n)
11831183
return offsetof(struct gen_ivtbl, as.shape.ivptr) + n * sizeof(VALUE);
11841184
}
11851185

1186-
static struct gen_ivtbl *
1187-
gen_ivtbl_resize(struct gen_ivtbl *old, uint32_t n)
1188-
{
1189-
RUBY_ASSERT(n > 0);
1190-
1191-
uint32_t len = old ? old->as.shape.numiv : 0;
1192-
struct gen_ivtbl *ivtbl = xrealloc(old, gen_ivtbl_bytes(n));
1193-
1194-
ivtbl->as.shape.numiv = n;
1195-
for (; len < n; len++) {
1196-
ivtbl->as.shape.ivptr[len] = Qundef;
1197-
}
1198-
1199-
return ivtbl;
1200-
}
1201-
12021186
void
12031187
rb_mark_generic_ivar(VALUE obj)
12041188
{
@@ -1651,51 +1635,55 @@ struct gen_ivar_lookup_ensure_size {
16511635
bool resize;
16521636
};
16531637

1654-
static int
1655-
generic_ivar_lookup_ensure_size(st_data_t *k, st_data_t *v, st_data_t u, int existing)
1656-
{
1657-
ASSERT_vm_locking();
1658-
1659-
struct gen_ivar_lookup_ensure_size *ivar_lookup = (struct gen_ivar_lookup_ensure_size *)u;
1660-
struct gen_ivtbl *ivtbl = existing ? (struct gen_ivtbl *)*v : NULL;
1661-
1662-
if (!existing || ivar_lookup->resize) {
1663-
if (existing) {
1664-
RUBY_ASSERT(ivar_lookup->shape->type == SHAPE_IVAR);
1665-
RUBY_ASSERT(rb_shape_get_shape_by_id(ivar_lookup->shape->parent_id)->capacity < ivar_lookup->shape->capacity);
1666-
}
1667-
else {
1668-
FL_SET_RAW((VALUE)*k, FL_EXIVAR);
1669-
}
1670-
1671-
ivtbl = gen_ivtbl_resize(ivtbl, ivar_lookup->shape->capacity);
1672-
*v = (st_data_t)ivtbl;
1673-
}
1674-
1675-
RUBY_ASSERT(FL_TEST((VALUE)*k, FL_EXIVAR));
1676-
1677-
ivar_lookup->ivtbl = ivtbl;
1678-
if (ivar_lookup->shape) {
1679-
#if SHAPE_IN_BASIC_FLAGS
1680-
rb_shape_set_shape(ivar_lookup->obj, ivar_lookup->shape);
1681-
#else
1682-
ivtbl->shape_id = rb_shape_id(ivar_lookup->shape);
1683-
#endif
1684-
}
1685-
1686-
return ST_CONTINUE;
1687-
}
1688-
16891638
static VALUE *
16901639
generic_ivar_set_shape_ivptr(VALUE obj, void *data)
16911640
{
16921641
RUBY_ASSERT(!rb_shape_obj_too_complex(obj));
16931642

16941643
struct gen_ivar_lookup_ensure_size *ivar_lookup = data;
16951644

1645+
// We can't use st_update, since when resizing the fields table GC can
1646+
// happen, which will modify the st_table and may rebuild it
16961647
RB_VM_LOCK_ENTER();
16971648
{
1698-
st_update(generic_ivtbl(obj, ivar_lookup->id, false), (st_data_t)obj, generic_ivar_lookup_ensure_size, (st_data_t)ivar_lookup);
1649+
struct gen_ivtbl *ivtbl = NULL;
1650+
st_table *tbl = generic_ivtbl(obj, ivar_lookup->id, false);
1651+
int existing = st_lookup(tbl, (st_data_t)obj, (st_data_t *)&ivtbl);
1652+
1653+
if (!existing || ivar_lookup->resize) {
1654+
uint32_t new_capa = ivar_lookup->shape->capacity;
1655+
uint32_t old_capa = rb_shape_get_shape_by_id(ivar_lookup->shape->parent_id)->capacity;
1656+
1657+
if (existing) {
1658+
RUBY_ASSERT(ivar_lookup->shape->type == SHAPE_IVAR);
1659+
RUBY_ASSERT(old_capa < new_capa);
1660+
RUBY_ASSERT(ivtbl);
1661+
} else {
1662+
RUBY_ASSERT(!ivtbl);
1663+
RUBY_ASSERT(old_capa == 0);
1664+
}
1665+
RUBY_ASSERT(new_capa > 0);
1666+
1667+
struct gen_ivtbl *old_ivtbl = ivtbl;
1668+
ivtbl = xmalloc(gen_ivtbl_bytes(new_capa));
1669+
if (old_ivtbl) {
1670+
memcpy(ivtbl, old_ivtbl, gen_ivtbl_bytes(old_capa));
1671+
}
1672+
ivtbl->as.shape.numiv = new_capa;
1673+
for (uint32_t i = old_capa; i < new_capa; i++) {
1674+
ivtbl->as.shape.ivptr[i] = Qundef;
1675+
}
1676+
1677+
st_insert(tbl, (st_data_t)obj, (st_data_t)ivtbl);
1678+
if (old_ivtbl) {
1679+
xfree(old_ivtbl);
1680+
}
1681+
}
1682+
1683+
ivar_lookup->ivtbl = ivtbl;
1684+
if (ivar_lookup->shape) {
1685+
rb_shape_set_shape(ivar_lookup->obj, ivar_lookup->shape);
1686+
}
16991687
}
17001688
RB_VM_LOCK_LEAVE();
17011689

@@ -2150,8 +2138,8 @@ rb_copy_generic_ivar(VALUE clone, VALUE obj)
21502138
new_ivtbl->as.complex.table = st_copy(obj_ivtbl->as.complex.table);
21512139
}
21522140
else {
2153-
new_ivtbl = gen_ivtbl_resize(0, obj_ivtbl->as.shape.numiv);
2154-
2141+
new_ivtbl = xmalloc(gen_ivtbl_bytes(obj_ivtbl->as.shape.numiv));
2142+
new_ivtbl->as.shape.numiv = obj_ivtbl->as.shape.numiv;
21552143
for (uint32_t i=0; i<obj_ivtbl->as.shape.numiv; i++) {
21562144
RB_OBJ_WRITE(clone, &new_ivtbl->as.shape.ivptr[i], obj_ivtbl->as.shape.ivptr[i]);
21572145
}

0 commit comments

Comments
 (0)
0