From 1a52c42e61878a1fe1d411a74108607766183b10 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Thu, 25 Sep 2025 18:30:12 -0400 Subject: [PATCH] Always use assert-free APIs when profiling and crashing rb_profile_frames() is used by profilers in a way such that it can run on any instruction in the binary, and it crashed previously in the following situation in `RUBY_DEBUG` builds: ``` * thread #1, queue = 'com.apple.main-thread', stop reason = step over frame #0: 0x00000001002827f0 miniruby`vm_make_env_each(ec=0x0000000101866b00, cfp=0x000000080c91bee8) at vm.c:992:74 989 } 990 991 vm_make_env_each(ec, prev_cfp); -> 992 VM_FORCE_WRITE_SPECIAL_CONST(&ep[VM_ENV_DATA_INDEX_SPECVAL], VM_GUARDED_PREV_EP(prev_cfp->ep)); 993 } 994 } 995 else { (lldb) call rb_profile_frames(0, 100, $2, $3) /Users/alan/ruby/vm_core.h:1448: Assertion Failed: VM_ENV_FLAGS:FIXNUM_P(flags) ruby 3.5.0dev (2025-09-23T20:20:04Z master 06b7a70837) +PRISM [arm64-darwin25] -- Crash Report log information -------------------------------------------- See Crash Report log file in one of the following locations: * ~/Library/Logs/DiagnosticReports * /Library/Logs/DiagnosticReports for more details. Don't forget to include the above Crash Report log file in bug reports. -- Control frame information ----------------------------------------------- c:0008 p:---- s:0029 e:000028 CFUNC :lambda /Users/alan/ruby/vm_core.h:1448: Assertion Failed: VM_ENV_FLAGS:FIXNUM_P(flags) ruby 3.5.0dev (2025-09-23T20:20:04Z master 06b7a70837) +PRISM [arm64-darwin25] -- Crash Report log information -------------------------------------------- ``` There is a small window where the control frame is invalid and fails the assert. The double crash also shows that in `RUBY_DEBUG` builds, the crash reporter was previously not resilient to corrupt frame state. In release builds, it prints more info. Add unchecked APIs for the crash reporter and profilers so they work as well in `RUBY_DEBUG` builds as non-debug builds. --- vm_backtrace.c | 8 +++++--- vm_core.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- vm_dump.c | 12 +++++++----- vm_insnhelper.c | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 9 deletions(-) diff --git a/vm_backtrace.c b/vm_backtrace.c index e81c568dda..07d2e33e32 100644 --- a/vm_backtrace.c +++ b/vm_backtrace.c @@ -1745,14 +1745,14 @@ thread_profile_frames(rb_execution_context_t *ec, int start, int limit, VALUE *b end_cfp = RUBY_VM_NEXT_CONTROL_FRAME(end_cfp); for (i=0; ipc != 0) { + if (VM_FRAME_RUBYFRAME_P_UNCHECKED(cfp) && cfp->pc != 0) { if (start > 0) { start--; continue; } /* record frame info */ - cme = rb_vm_frame_method_entry(cfp); + cme = rb_vm_frame_method_entry_unchecked(cfp); if (cme && cme->def->type == VM_METHOD_TYPE_ISEQ) { buff[i] = (VALUE)cme; } @@ -1770,6 +1770,8 @@ thread_profile_frames(rb_execution_context_t *ec, int start, int limit, VALUE *b // before entering a non-leaf method (so that `caller` will work), // so only the topmost frame could possibly have an out-of-date PC. // ZJIT doesn't set `cfp->jit_return`, so it's not a reliable signal. + // TODO(zjit): lightweight frames potentially makes more than + // the top most frame invalid. // // Avoid passing invalid PC to calc_lineno() to avoid crashing. if (cfp == top && (pc < iseq_encoded || pc > pc_end)) { @@ -1783,7 +1785,7 @@ thread_profile_frames(rb_execution_context_t *ec, int start, int limit, VALUE *b i++; } else { - cme = rb_vm_frame_method_entry(cfp); + cme = rb_vm_frame_method_entry_unchecked(cfp); if (cme && cme->def->type == VM_METHOD_TYPE_CFUNC) { if (start > 0) { start--; diff --git a/vm_core.h b/vm_core.h index 2e77e1073e..487a4020ee 100644 --- a/vm_core.h +++ b/vm_core.h @@ -1449,12 +1449,25 @@ VM_ENV_FLAGS(const VALUE *ep, long flag) return flags & flag; } +static inline unsigned long +VM_ENV_FLAGS_UNCHECKED(const VALUE *ep, long flag) +{ + VALUE flags = ep[VM_ENV_DATA_INDEX_FLAGS]; + return flags & flag; +} + static inline unsigned long VM_FRAME_TYPE(const rb_control_frame_t *cfp) { return VM_ENV_FLAGS(cfp->ep, VM_FRAME_MAGIC_MASK); } +static inline unsigned long +VM_FRAME_TYPE_UNCHECKED(const rb_control_frame_t *cfp) +{ + return VM_ENV_FLAGS_UNCHECKED(cfp->ep, VM_FRAME_MAGIC_MASK); +} + static inline int VM_FRAME_LAMBDA_P(const rb_control_frame_t *cfp) { @@ -1473,6 +1486,12 @@ VM_FRAME_FINISHED_P(const rb_control_frame_t *cfp) return VM_ENV_FLAGS(cfp->ep, VM_FRAME_FLAG_FINISH) != 0; } +static inline int +VM_FRAME_FINISHED_P_UNCHECKED(const rb_control_frame_t *cfp) +{ + return VM_ENV_FLAGS_UNCHECKED(cfp->ep, VM_FRAME_FLAG_FINISH) != 0; +} + static inline int VM_FRAME_BMETHOD_P(const rb_control_frame_t *cfp) { @@ -1498,12 +1517,24 @@ VM_FRAME_CFRAME_P(const rb_control_frame_t *cfp) return cframe_p; } +static inline int +VM_FRAME_CFRAME_P_UNCHECKED(const rb_control_frame_t *cfp) +{ + return VM_ENV_FLAGS_UNCHECKED(cfp->ep, VM_FRAME_FLAG_CFRAME) != 0; +} + static inline int VM_FRAME_RUBYFRAME_P(const rb_control_frame_t *cfp) { return !VM_FRAME_CFRAME_P(cfp); } +static inline int +VM_FRAME_RUBYFRAME_P_UNCHECKED(const rb_control_frame_t *cfp) +{ + return !VM_FRAME_CFRAME_P_UNCHECKED(cfp); +} + static inline int VM_FRAME_NS_SWITCH_P(const rb_control_frame_t *cfp) { @@ -1522,11 +1553,23 @@ VM_ENV_LOCAL_P(const VALUE *ep) return VM_ENV_FLAGS(ep, VM_ENV_FLAG_LOCAL) ? 1 : 0; } +static inline int +VM_ENV_LOCAL_P_UNCHECKED(const VALUE *ep) +{ + return VM_ENV_FLAGS_UNCHECKED(ep, VM_ENV_FLAG_LOCAL) ? 1 : 0; +} + +static inline const VALUE * +VM_ENV_PREV_EP_UNCHECKED(const VALUE *ep) +{ + return GC_GUARDED_PTR_REF(ep[VM_ENV_DATA_INDEX_SPECVAL]); +} + static inline const VALUE * VM_ENV_PREV_EP(const VALUE *ep) { VM_ASSERT(VM_ENV_LOCAL_P(ep) == 0); - return GC_GUARDED_PTR_REF(ep[VM_ENV_DATA_INDEX_SPECVAL]); + return VM_ENV_PREV_EP_UNCHECKED(ep); } static inline VALUE @@ -1934,6 +1977,7 @@ void rb_gc_mark_machine_context(const rb_execution_context_t *ec); rb_cref_t *rb_vm_rewrite_cref(rb_cref_t *node, VALUE old_klass, VALUE new_klass); const rb_callable_method_entry_t *rb_vm_frame_method_entry(const rb_control_frame_t *cfp); +const rb_callable_method_entry_t *rb_vm_frame_method_entry_unchecked(const rb_control_frame_t *cfp); #define sysstack_error GET_VM()->special_exceptions[ruby_error_sysstack] diff --git a/vm_dump.c b/vm_dump.c index 2a863ddef9..131844b4cc 100644 --- a/vm_dump.c +++ b/vm_dump.c @@ -61,14 +61,14 @@ control_frame_dump(const rb_execution_context_t *ec, const rb_control_frame_t *c const char *magic, *iseq_name = "-", *selfstr = "-", *biseq_name = "-"; VALUE tmp; const rb_iseq_t *iseq = NULL; - const rb_callable_method_entry_t *me = rb_vm_frame_method_entry(cfp); + const rb_callable_method_entry_t *me = rb_vm_frame_method_entry_unchecked(cfp); if (ep < 0 || (size_t)ep > ec->vm_stack_size) { ep = (ptrdiff_t)cfp->ep; ep_in_heap = 'p'; } - switch (VM_FRAME_TYPE(cfp)) { + switch (VM_FRAME_TYPE_UNCHECKED(cfp)) { case VM_FRAME_MAGIC_TOP: magic = "TOP"; break; @@ -128,7 +128,9 @@ control_frame_dump(const rb_execution_context_t *ec, const rb_control_frame_t *c iseq = cfp->iseq; pc = cfp->pc - ISEQ_BODY(iseq)->iseq_encoded; iseq_name = RSTRING_PTR(ISEQ_BODY(iseq)->location.label); - line = rb_vm_get_sourceline(cfp); + if (pc >= 0 && pc <= ISEQ_BODY(iseq)->iseq_size) { + line = rb_vm_get_sourceline(cfp); + } if (line) { snprintf(posbuf, MAX_POSBUF, "%s:%d", RSTRING_PTR(rb_iseq_path(iseq)), line); } @@ -138,7 +140,7 @@ control_frame_dump(const rb_execution_context_t *ec, const rb_control_frame_t *c } } } - else if (me != NULL) { + else if (me != NULL && IMEMO_TYPE_P(me, imemo_ment)) { iseq_name = rb_id2name(me->def->original_id); snprintf(posbuf, MAX_POSBUF, ":%s", iseq_name); line = -1; @@ -158,7 +160,7 @@ control_frame_dump(const rb_execution_context_t *ec, const rb_control_frame_t *c if (line) { kprintf(" %s", posbuf); } - if (VM_FRAME_FINISHED_P(cfp)) { + if (VM_FRAME_FINISHED_P_UNCHECKED(cfp)) { kprintf(" [FINISH]"); } if (0) { diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 8022a29a6e..75339aaa5c 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -761,6 +761,25 @@ check_method_entry(VALUE obj, int can_be_svar) } } +static rb_callable_method_entry_t * +env_method_entry_unchecked(VALUE obj, int can_be_svar) +{ + if (obj == Qfalse) return NULL; + + switch (imemo_type(obj)) { + case imemo_ment: + return (rb_callable_method_entry_t *)obj; + case imemo_cref: + return NULL; + case imemo_svar: + if (can_be_svar) { + return env_method_entry_unchecked(((struct vm_svar *)obj)->cref_or_me, FALSE); + } + default: + return NULL; + } +} + const rb_callable_method_entry_t * rb_vm_frame_method_entry(const rb_control_frame_t *cfp) { @@ -775,6 +794,20 @@ rb_vm_frame_method_entry(const rb_control_frame_t *cfp) return check_method_entry(ep[VM_ENV_DATA_INDEX_ME_CREF], TRUE); } +const rb_callable_method_entry_t * +rb_vm_frame_method_entry_unchecked(const rb_control_frame_t *cfp) +{ + const VALUE *ep = cfp->ep; + rb_callable_method_entry_t *me; + + while (!VM_ENV_LOCAL_P_UNCHECKED(ep)) { + if ((me = env_method_entry_unchecked(ep[VM_ENV_DATA_INDEX_ME_CREF], FALSE)) != NULL) return me; + ep = VM_ENV_PREV_EP_UNCHECKED(ep); + } + + return env_method_entry_unchecked(ep[VM_ENV_DATA_INDEX_ME_CREF], TRUE); +} + static const rb_iseq_t * method_entry_iseqptr(const rb_callable_method_entry_t *me) {