Skip to content

Commit 59d6fed

Browse files
luke-gruberk0kubun
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] (Backport 4.0)
1 parent 48ac726 commit 59d6fed

3 files changed

Lines changed: 19 additions & 6 deletions

File tree

imemo.c

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -577,12 +577,7 @@ rb_imemo_free(VALUE obj)
577577
break;
578578
case imemo_callinfo:{
579579
const struct rb_callinfo *ci = ((const struct rb_callinfo *)obj);
580-
581-
if (ci->kwarg) {
582-
if (RUBY_ATOMIC_FETCH_SUB(((struct rb_callinfo_kwarg *)ci->kwarg)->references, 1) == 1) {
583-
xfree((void *)ci->kwarg);
584-
}
585-
}
580+
rb_callinfo_kwarg_release((struct rb_callinfo_kwarg *)ci->kwarg);
586581
RB_DEBUG_COUNTER_INC(obj_imemo_callinfo);
587582

588583
break;

prism_compile.c

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

3779+
rb_callinfo_kwarg_retain(kw_arg);
3780+
37793781
// Jump unless the receiver uses the "basic" implementation of "new"
37803782
VALUE ci;
37813783
if (flags & VM_CALL_FORWARDING) {
@@ -3798,6 +3800,8 @@ pm_compile_call(rb_iseq_t *iseq, const pm_call_node_t *call_node, LINK_ANCHOR *c
37983800

37993801
PUSH_LABEL(ret, not_basic_new_finish);
38003802
PUSH_INSN(ret, location, pop);
3803+
3804+
rb_callinfo_kwarg_release(kw_arg);
38013805
}
38023806
else {
38033807
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_sized_xfree(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)