Skip to content

Commit 1ca2d2d

Browse files
luke-gruberluke-gru
authored andcommitted
Fix pm_compile_call for inline_new with keyword args
pm_setup_args mallocs one kw_arg buffer with references = 0. The inline_new branch then feeds that same pointer to three callinfo constructions: - 3789 — new_callinfo(... method_id="new" ..., kw_arg, 0) -> the opt_new ci - 3795 — PUSH_SEND_R(... "initialize", ..., flags | VM_CALL_FCALL, kw_arg) - 3800 — PUSH_SEND_R(... method_id="new", ..., flags, kw_arg) (fallback) Tracing the refcount through rb_vm_ci_lookup() 1. It increments kwarg->references and allocates a fresh new_ci before the dedup st_update 2. The dedup (vm_ci_hash_cmp) compares kwarg contents, not the pointer. So if an earlier line already interned a new ci with the identical keyword set, st_update returns that pre-existing ci (which holds a different buffer) and discards our new_ci. 3. Our kw_arg is now orphaned: references == 1, but the only holder is the discarded new_ci, which is a normal collectable imemo 4. An allocation like PUSH_INSN2 opt_new at or new_callinfo() can trigger a GC. References back to 0, kw_arg buffer freed. 5. new_callinfo() using the freed buffer, does argc += kw_arg->keyword_len (use-after-free) The fix: Keep the buffer alive across the allocations in inline_new. Fixes [Bug #22104]
1 parent 2a23415 commit 1ca2d2d

3 files changed

Lines changed: 19 additions & 5 deletions

File tree

imemo.c

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -645,11 +645,7 @@ rb_imemo_free(VALUE obj)
645645
case imemo_callinfo:{
646646
const struct rb_callinfo *ci = ((const struct rb_callinfo *)obj);
647647

648-
if (ci->kwarg) {
649-
if (RUBY_ATOMIC_FETCH_SUB(((struct rb_callinfo_kwarg *)ci->kwarg)->references, 1) == 1) {
650-
ruby_xfree_sized((void *)ci->kwarg, rb_callinfo_kwarg_bytes(ci->kwarg->keyword_len));
651-
}
652-
}
648+
rb_callinfo_kwarg_release((struct rb_callinfo_kwarg *)ci->kwarg);
653649
RB_DEBUG_COUNTER_INC(obj_imemo_callinfo);
654650

655651
break;

prism_compile.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3887,6 +3887,8 @@ pm_compile_call(rb_iseq_t *iseq, const pm_call_node_t *call_node, LINK_ANCHOR *c
38873887
ELEM_INSERT_NEXT(opt_new_prelude, &new_insn_body(iseq, location.line, location.node_id, BIN(putnil), 0)->link);
38883888
}
38893889

3890+
rb_callinfo_kwarg_retain(kw_arg);
3891+
38903892
// Jump unless the receiver uses the "basic" implementation of "new"
38913893
VALUE ci;
38923894
if (flags & VM_CALL_FORWARDING) {
@@ -3909,6 +3911,8 @@ pm_compile_call(rb_iseq_t *iseq, const pm_call_node_t *call_node, LINK_ANCHOR *c
39093911

39103912
PUSH_LABEL(ret, not_basic_new_finish);
39113913
PUSH_INSN(ret, location, pop);
3914+
3915+
rb_callinfo_kwarg_release(kw_arg);
39123916
}
39133917
else {
39143918
PUSH_SEND_R(ret, location, method_id, INT2FIX(orig_argc), block_iseq, INT2FIX(flags), kw_arg);

vm_callinfo.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,20 @@ rb_callinfo_kwarg_bytes(int keyword_len)
6161
rb_eRuntimeError);
6262
}
6363

64+
static inline void
65+
rb_callinfo_kwarg_retain(struct rb_callinfo_kwarg *kwarg)
66+
{
67+
if (kwarg) RUBY_ATOMIC_INC(kwarg->references);
68+
}
69+
70+
static inline void
71+
rb_callinfo_kwarg_release(struct rb_callinfo_kwarg *kwarg)
72+
{
73+
if (kwarg && RUBY_ATOMIC_FETCH_SUB(kwarg->references, 1) == 1) {
74+
ruby_xfree_sized(kwarg, rb_callinfo_kwarg_bytes(kwarg->keyword_len));
75+
}
76+
}
77+
6478
// imemo_callinfo
6579
struct rb_callinfo {
6680
VALUE flags;

0 commit comments

Comments
 (0)