Skip to content

Commit 3dd77bc

Browse files
committed
Fix corruption when out of shape during ivar remove
Reproduction script: ``` o = Object.new 10.times { |i| o.instance_variable_set(:"@A#{i}", i) } i = 0 a = Object.new while RubyVM::Shape.shapes_available > 2 a.instance_variable_set(:"@i#{i}", 1) i += 1 end o.remove_instance_variable(:@a0) puts o.instance_variable_get(:@A1) ``` Before this patch, it would incorrectly output `2` and now it correctly outputs `1`.
1 parent 7c99e43 commit 3dd77bc

2 files changed

Lines changed: 55 additions & 49 deletions

File tree

shape.c

Lines changed: 34 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -528,29 +528,8 @@ rb_shape_frozen_shape_p(rb_shape_t* shape)
528528
return SHAPE_FROZEN == (enum shape_type)shape->type;
529529
}
530530

531-
static void
532-
move_iv(VALUE obj, ID id, attr_index_t from, attr_index_t to)
533-
{
534-
switch(BUILTIN_TYPE(obj)) {
535-
case T_CLASS:
536-
case T_MODULE:
537-
RCLASS_IVPTR(obj)[to] = RCLASS_IVPTR(obj)[from];
538-
break;
539-
case T_OBJECT:
540-
RUBY_ASSERT(!rb_shape_obj_too_complex(obj));
541-
ROBJECT_IVPTR(obj)[to] = ROBJECT_IVPTR(obj)[from];
542-
break;
543-
default: {
544-
struct gen_ivtbl *ivtbl;
545-
rb_gen_ivtbl_get(obj, id, &ivtbl);
546-
ivtbl->as.shape.ivptr[to] = ivtbl->as.shape.ivptr[from];
547-
break;
548-
}
549-
}
550-
}
551-
552531
static rb_shape_t *
553-
remove_shape_recursive(VALUE obj, ID id, rb_shape_t * shape, VALUE * removed)
532+
remove_shape_recursive(rb_shape_t *shape, ID id, rb_shape_t **removed_shape)
554533
{
555534
if (shape->parent_id == INVALID_SHAPE_ID) {
556535
// We've hit the top of the shape tree and couldn't find the
@@ -559,30 +538,13 @@ remove_shape_recursive(VALUE obj, ID id, rb_shape_t * shape, VALUE * removed)
559538
}
560539
else {
561540
if (shape->type == SHAPE_IVAR && shape->edge_name == id) {
562-
// We've hit the edge we wanted to remove, return it's _parent_
563-
// as the new parent while we go back down the stack.
564-
attr_index_t index = shape->next_iv_index - 1;
565-
566-
switch(BUILTIN_TYPE(obj)) {
567-
case T_CLASS:
568-
case T_MODULE:
569-
*removed = RCLASS_IVPTR(obj)[index];
570-
break;
571-
case T_OBJECT:
572-
*removed = ROBJECT_IVPTR(obj)[index];
573-
break;
574-
default: {
575-
struct gen_ivtbl *ivtbl;
576-
rb_gen_ivtbl_get(obj, id, &ivtbl);
577-
*removed = ivtbl->as.shape.ivptr[index];
578-
break;
579-
}
580-
}
541+
*removed_shape = shape;
542+
581543
return rb_shape_get_parent(shape);
582544
}
583545
else {
584546
// This isn't the IV we want to remove, keep walking up.
585-
rb_shape_t * new_parent = remove_shape_recursive(obj, id, rb_shape_get_parent(shape), removed);
547+
rb_shape_t *new_parent = remove_shape_recursive(rb_shape_get_parent(shape), id, removed_shape);
586548

587549
// We found a new parent. Create a child of the new parent that
588550
// has the same attributes as this shape.
@@ -592,17 +554,13 @@ remove_shape_recursive(VALUE obj, ID id, rb_shape_t * shape, VALUE * removed)
592554
}
593555

594556
bool dont_care;
595-
rb_shape_t * new_child = get_next_shape_internal(new_parent, shape->edge_name, shape->type, &dont_care, true);
557+
rb_shape_t *new_child = get_next_shape_internal(new_parent, shape->edge_name, shape->type, &dont_care, true);
596558
if (UNLIKELY(new_child->type == SHAPE_OBJ_TOO_COMPLEX)) {
597559
return new_child;
598560
}
599561

600562
RUBY_ASSERT(new_child->capacity <= shape->capacity);
601563

602-
if (new_child->type == SHAPE_IVAR) {
603-
move_iv(obj, id, shape->next_iv_index - 1, new_child->next_iv_index - 1);
604-
}
605-
606564
return new_child;
607565
}
608566
else {
@@ -615,18 +573,45 @@ remove_shape_recursive(VALUE obj, ID id, rb_shape_t * shape, VALUE * removed)
615573
}
616574

617575
bool
618-
rb_shape_transition_shape_remove_ivar(VALUE obj, ID id, rb_shape_t *shape, VALUE * removed)
576+
rb_shape_transition_shape_remove_ivar(VALUE obj, ID id, rb_shape_t *shape, VALUE *removed)
619577
{
620578
if (UNLIKELY(shape->type == SHAPE_OBJ_TOO_COMPLEX)) {
621579
return false;
622580
}
623581

624-
rb_shape_t * new_shape = remove_shape_recursive(obj, id, shape, removed);
582+
rb_shape_t *removed_shape = NULL;
583+
rb_shape_t *new_shape = remove_shape_recursive(shape, id, &removed_shape);
625584
if (new_shape) {
585+
RUBY_ASSERT(removed_shape != NULL);
586+
626587
if (UNLIKELY(new_shape->type == SHAPE_OBJ_TOO_COMPLEX)) {
627588
return false;
628589
}
629590

591+
RUBY_ASSERT(new_shape->next_iv_index == shape->next_iv_index - 1);
592+
593+
VALUE *ivptr;
594+
switch(BUILTIN_TYPE(obj)) {
595+
case T_CLASS:
596+
case T_MODULE:
597+
ivptr = RCLASS_IVPTR(obj);
598+
break;
599+
case T_OBJECT:
600+
ivptr = ROBJECT_IVPTR(obj);
601+
break;
602+
default: {
603+
struct gen_ivtbl *ivtbl;
604+
rb_gen_ivtbl_get(obj, id, &ivtbl);
605+
ivptr = ivtbl->as.shape.ivptr;
606+
break;
607+
}
608+
}
609+
610+
*removed = ivptr[removed_shape->next_iv_index - 1];
611+
612+
memmove(&ivptr[removed_shape->next_iv_index - 1], &ivptr[removed_shape->next_iv_index],
613+
((new_shape->next_iv_index + 1) - removed_shape->next_iv_index) * sizeof(VALUE));
614+
630615
rb_shape_set_shape(obj, new_shape);
631616
}
632617
return true;

test/ruby/test_shapes.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,27 @@ module A
414414
assert_equal true, A.instance_variable_defined?(:@a)
415415
end;
416416
end
417+
418+
def test_run_out_of_shape_during_remove_instance_variable
419+
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
420+
begin;
421+
o = Object.new
422+
10.times { |i| o.instance_variable_set(:"@a#{i}", i) }
423+
424+
i = 0
425+
a = Object.new
426+
while RubyVM::Shape.shapes_available > 2
427+
a.instance_variable_set(:"@i#{i}", 1)
428+
i += 1
429+
end
430+
431+
o.remove_instance_variable(:@a0)
432+
(1...10).each do |i|
433+
assert_equal(i, o.instance_variable_get(:"@a#{i}"))
434+
end
435+
end;
436+
end
437+
417438
def test_run_out_of_shape_remove_instance_variable
418439
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
419440
begin;

0 commit comments

Comments
 (0)