Avoid the impact of object modifications caused by calls to mrb_vm_exec()#6756
Avoid the impact of object modifications caused by calls to mrb_vm_exec()#6756dearblue wants to merge 1 commit intomruby:masterfrom
mrb_vm_exec()#6756Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical memory safety vulnerabilities in mruby's array extension methods. By strategically applying garbage collection protection mechanisms and explicitly marking array modifications, it prevents potential use-after-free errors and other undefined behaviors that could occur when C-implemented array operations interact with Ruby-side object modifications or garbage collection cycles. The changes enhance the robustness and stability of array handling within the mruby runtime. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses critical memory safety issues in array extension methods by preventing object modifications during C-level iterations from causing use-after-free errors or other undefined behavior. The changes consistently apply garbage collection protection to array elements and avoid caching array lengths in loops where Ruby code execution could modify the array. The approach is sound and the fixes are comprehensive. However, I've identified one critical bug in the implementation for ary_intersect_p where an arena restore call is misplaced, which could lead to a use-after-free vulnerability. With that fix, this PR will be a great improvement to the stability of mruby.
mrbgems/mruby-array-ext/src/array.c
Outdated
| mrb_gc_arena_restore(mrb, ai); | ||
| } |
There was a problem hiding this comment.
The mrb_gc_arena_restore(mrb, ai) call is inside the inner for loop. This will cause p, which was protected before the outer loop, to become unprotected after the first iteration of this inner loop. If a subsequent call to mrb_equal within the same inner loop triggers a garbage collection, it could lead to a use-after-free on p.
The arena restoration should happen after the inner loop has completed to ensure p remains protected throughout all inner loop iterations.
}
mrb_gc_arena_restore(mrb, ai);
…xec()` Several methods defined in mruby-array-ext are written in C and may call `mrb_vm_exec()`. If array objects are modified on the Ruby side, problems may arise in subsequent processing. - Using objects that have been removed from the array and garbage collected - Using pointers or array lengths that have become invalid due to changes to the array object - Modifying the contents of a shared array object directly ref: mruby#6662
1 participant
Several methods defined in mruby-array-ext are written in C and may call
mrb_vm_exec().If array objects are modified on the Ruby side, problems may arise in subsequent processing.
ref: #6662
This patch may be excessive, or it may not be comprehensive enough.
I’ve attached two examples of code that reproduce the issue, along with the results.
Use-after-free in objects caused by `Array#assoc`
Use-after-free in objects caused by `Array#|`