Skip to content

Commit

Permalink
Make expandarray compaction safe
Browse files Browse the repository at this point in the history
The expandarray instruction can allocate an array, which can trigger
a GC compaction. However, since it does not increment the sp until the
end of the instruction, the objects it places on the stack are not
marked or reference updated by the GC, which can cause the objects to
move which leaves broken or incorrect objects on the stack.

This commit changes the instruction to be handles_sp so the sp is
incremented inside of the instruction right after the object is written
on the stack.
  • Loading branch information
peterzhu2118 committed Dec 1, 2023
1 parent 492c82c commit 0aed37b
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 24 deletions.
3 changes: 2 additions & 1 deletion insns.def
Expand Up @@ -498,10 +498,11 @@ expandarray
(rb_num_t num, rb_num_t flag)
(..., VALUE ary)
(...)
// attr bool handles_sp = true;
// attr bool leaf = false; /* has rb_check_array_type() */
// attr rb_snum_t sp_inc = (rb_snum_t)num - 1 + (flag & 1 ? 1 : 0);
{
vm_expandarray(GET_SP(), ary, num, (int)flag);
vm_expandarray(GET_CFP(), ary, num, (int)flag);
}

/* concat two arrays */
Expand Down
51 changes: 28 additions & 23 deletions vm_insnhelper.c
Expand Up @@ -1866,11 +1866,9 @@ rb_vm_throw(const rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, rb_nu
}

static inline void
vm_expandarray(VALUE *sp, VALUE ary, rb_num_t num, int flag)
vm_expandarray(struct rb_control_frame_struct *cfp, VALUE ary, rb_num_t num, int flag)
{
int is_splat = flag & 0x01;
rb_num_t space_size = num + is_splat;
VALUE *base = sp - 1;
const VALUE *ptr;
rb_num_t len;
const VALUE obj = ary;
Expand All @@ -1885,49 +1883,56 @@ vm_expandarray(VALUE *sp, VALUE ary, rb_num_t num, int flag)
len = (rb_num_t)RARRAY_LEN(ary);
}

if (space_size == 0) {
if (num + is_splat == 0) {
/* no space left on stack */
}
else if (flag & 0x02) {
/* post: ..., nil ,ary[-1], ..., ary[0..-num] # top */
rb_num_t i = 0, j;

if (len < num) {
for (i=0; i<num-len; i++) {
*base++ = Qnil;
for (i = 0; i < num - len; i++) {
*cfp->sp++ = Qnil;
}
}
for (j=0; i<num; i++, j++) {

for (j = 0; i < num; i++, j++) {
VALUE v = ptr[len - j - 1];
*base++ = v;
*cfp->sp++ = v;
}

if (is_splat) {
*base = rb_ary_new4(len - j, ptr);
*cfp->sp++ = rb_ary_new4(len - j, ptr);
}
}
else {
/* normal: ary[num..-1], ary[num-2], ary[num-3], ..., ary[0] # top */
rb_num_t i;
VALUE *bptr = &base[space_size - 1];

for (i=0; i<num; i++) {
if (len <= i) {
for (; i<num; i++) {
*bptr-- = Qnil;
}
break;
}
*bptr-- = ptr[i];
}
if (is_splat) {
if (num > len) {
*bptr = rb_ary_new();
*cfp->sp++ = rb_ary_new();
}
else {
*bptr = rb_ary_new4(len - num, ptr + num);
*cfp->sp++ = rb_ary_new4(len - num, ptr + num);
}
}

if (num > len) {
rb_num_t i = 0;
for (; i < num - len; i++) {
*cfp->sp++ = Qnil;
}

for (rb_num_t j = 0; i < num; i++, j++) {
*cfp->sp++ = ptr[len - j - 1];
}
}
else {
for (rb_num_t j = 0; j < num; j++) {
*cfp->sp++ = ptr[num - j - 1];
}
}
}

RB_GC_GUARD(ary);
}

Expand Down

0 comments on commit 0aed37b

Please sign in to comment.