Add alias support for mruby-method and mruby-proc-ext#6879
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes potential segmentation faults and misaligned reads by resolving alias procs to their underlying original procs before accessing their properties in mrb_proc_parameters and mrb_proc_arity. It also adds comprehensive test coverage for aliased and C-defined methods. The review feedback correctly identifies that several other functions, such as proc_inspect, mrb_proc_local_variables, and mrb_proc_copy, still access body.irep directly without resolving aliases, which could lead to similar crashes or memory corruption. It is highly recommended to apply the same alias resolution guards to these functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| while (MRB_PROC_ALIAS_P(proc)) { | ||
| proc = proc->upper; | ||
| if (!proc) return mrb_ary_new(mrb); | ||
| } |
There was a problem hiding this comment.
While resolving aliases in mrb_proc_parameters successfully prevents a SEGV here, proc_inspect (lines 103–132) in the same file suffers from the exact same vulnerability. In proc_inspect, if !MRB_PROC_CFUNC_P(p) is true, it directly accesses p->body.irep and passes it to mrb_debug_get_position. For an alias proc, body.irep is actually body.mid (a symbol), which will cause a misaligned read or SEGV when dereferenced.
Please consider resolving aliases in proc_inspect as well, for example:
while (p && MRB_PROC_ALIAS_P(p)) {
p = p->upper;
}| while (p && MRB_PROC_ALIAS_P(p)) { | ||
| p = p->upper; | ||
| } | ||
| if (!p) return 0; |
There was a problem hiding this comment.
Resolving aliases in mrb_proc_arity is a great fix. However, there are other functions in src/proc.c that access proc->body.irep directly without resolving aliases:
mrb_proc_local_variables(lines 469–498) accessesproc->body.irepdirectly in its loop. If an alias proc is passed, it will treatbody.mid(a symbol) as anmrb_irep*and dereferenceirep->lv, leading to a SEGV.mrb_proc_copy(lines 276–290) accessesb->body.irepand callsmrb_irep_increfon it if!MRB_PROC_CFUNC_P(b)is true. For an alias proc, this will corrupt memory by incrementing a reference count on a symbol value.
Please consider adding alias resolution guards to mrb_proc_local_variables and mrb_proc_copy to prevent these potential crashes and memory corruptions.
|
@matz didn't see the issues in the other functions gemini flags. Wouldn't a global helper method be better here for accessing this which does the alias resolution itself? |
|
Thanks, this is a clean fix and the tests are thorough. Confirmed the segfault on master and that this resolves it. On the helper idea: yes, the |
Ive hit a Segfault when using mruby-method with an aliased C mruby function definition.
This fixed it, i also added some tests to verify the fix works.