vm.c: refresh regs before storing the OP_GETIDX0 Hash result - #7023

The bug

vm_op_getidx0() stores the Hash result through the registers it came in with:

  else if (tt == MRB_TT_HASH) {
    if (mrb_obj_ptr(recv)->c != mrb->hash_class) goto getidx0_fallback;
    regs[a] = mrb_hash_get(mrb, recv, mrb_fixnum_value(0));
    return VM_NEXT;
  }

regs is #define regs (ci->stack). mrb_hash_get() runs the default proc when the key is missing. For a plain Hash.new { ... } the default method is still the builtin, so mrb_func_basic_p() succeeds and the work goes to hash_default(), which calls the proc through mrb_funcall_argv2(); the mrb_funcall_argv() at the end of mrb_hash_get() is the other route into the same place, taken when default has been overridden. Either way the VM is re-entered, and a proc that pushes enough frames reaches stack_extend(), which reallocates c->stbase and frees the old buffer.

The destination of the store is ci->stack + a, and the C evaluation order for an assignment is unspecified, so the address is computed on the way in and the write lands in the freed buffer. A ci refresh cannot be spliced into that single statement either, so the value has to pass through a local first, which is what puts the refresh before the store.

Reproducing

def deep(n)
  return 0 if n == 0
  deep(n - 1)
end

h = Hash.new { |hash, key| deep(50); :from_proc }

def probe(h)
  v = h[0]
  v
end

p probe(h)

Built with build_config/clang-asan.rb:

==614957==ERROR: AddressSanitizer: heap-use-after-free on address 0x70412fbe00b8
WRITE of size 8 at 0x70412fbe00b8 thread T0
    #1 vm_op_getidx0 src/vm.c:2127:15
    #2 mrb_vm_exec   src/vm.c:2555:15
freed by thread T0 here:
    #4 stack_extend_alloc     src/vm.c:209:37
    #5 stack_extend           src/vm.c:226:5
    #6 mrb_vm_exec            src/vm.c:2934:11
    #9 mrb_funcall_with_block src/vm.c:878:13
    #10 mrb_funcall_argv      src/vm.c:906:10
    #11 mrb_funcall_argv2     include/mruby.h:1235:10
    #12 hash_default          src/hash.c:1314:14
    #13 mrb_hash_get          src/hash.c:1403:12
    #14 vm_op_getidx0         src/vm.c:2127:15
    #15 mrb_vm_exec           src/vm.c:2555:15

The frame that frees the buffer is the default proc's own VM run, so the free and the write both belong to the one h[0].

Changing h[0] to h[1] moves the work to OP_GETIDX, and that one prints :from_proc and exits 0 under the same sanitizer. So what the two branches differ by is the refresh, not the default proc.

deep(50) is the whole requirement. CALLINFO_INIT_SIZE is 32, so fifty frames also reallocate c->cibase, which leaves the helper's own ci pointing into a freed array as well. The sanitizer reports the stack buffer first because that is what the store touches, but both are stale.

The fix

Take the result into a local, refresh ci, then store, which is what vm_op_getidx() and both branches of vm_op_setidx() already do. The ci = mrb->c->ci; in the CASE(OP_GETIDX0) arm refreshes the caller's copy after the helper has returned, which is too late for a store inside it.

This is the shape 7b503f3a3 ("vm.c: store through the refreshed regs after a VM re-entry") fixed for OP_GETIV and OP_ARYSPLAT. vm_op_getidx0() was not covered there.

The test

test/t/hash.rb gets an assertion over both h[0] and h[1], so it covers the branch that is broken and the branch that is the model for the fix. It asserts the value rather than the memory error, since the abandoned buffer usually still holds something plausible: on an unsanitized build the unfixed VM prints :from_proc and exits 0 all the same. The sanitizer build is what makes the failure deterministic, and build_config/clang-asan.rb already runs rake test.

Overlap with #7022

#7022 puts an arena save and restore around these same three lines. The two changes are independent in effect, the refresh does not change what the arena does and the restore does not change where the store goes, but they do touch the same lines, so whichever lands second wants a trivial resolve. The combined form is:

    int ai = mrb_gc_arena_save(mrb);
    mrb_value val = mrb_hash_get(mrb, recv, mrb_fixnum_value(0));
    ci = mrb->c->ci;
    regs[a] = val;
    mrb_gc_arena_restore(mrb, ai);

This branches from 0b4e3f15d rather than from #7022 so that it can be read and merged on its own.

Summary by CodeRabbit

• Bug Fixes

  • Fixed hash lookups with default procedures during deep recursive calls, preventing incorrect results when the runtime stack grows.
  • Hash indexing now reliably returns the expected default value in these scenarios.

• Tests

  • Added regression coverage for hash access that recursively re-enters the runtime.