From a687756284187887835aa345adc89b2718054e4a Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Fri, 29 Apr 2022 18:54:16 -0400 Subject: Fix use-after-free with interacting TracePoints `vm_trace_hook()` runs global hooks before running local hooks. Previously, we read the local hook list before running the global hooks which led to use-after-free when a global hook frees the local hook list. A global hook can do this by disabling a local TracePoint, for example. Delay local hook list loading until after running the global hooks. Issue discovered by Jeremy Evans in GH-5862. [Bug #18730] --- vm_insnhelper.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'vm_insnhelper.c') diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 1e40088ffa..09fcd2f729 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -5629,7 +5629,7 @@ NOINLINE(static void vm_trace(rb_execution_context_t *ec, rb_control_frame_t *re static inline void vm_trace_hook(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, const VALUE *pc, rb_event_flag_t pc_events, rb_event_flag_t target_event, - rb_hook_list_t *global_hooks, rb_hook_list_t *local_hooks, VALUE val) + rb_hook_list_t *global_hooks, rb_hook_list_t *const *local_hooks_ptr, VALUE val) { rb_event_flag_t event = pc_events & target_event; VALUE self = GET_SELF(); @@ -5644,6 +5644,8 @@ vm_trace_hook(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, const VAL reg_cfp->pc--; } + // Load here since global hook above can add and free local hooks + rb_hook_list_t *local_hooks = *local_hooks_ptr; if (local_hooks != NULL) { if (event & local_hooks->events) { /* increment PC because source line is calculated with PC-1 */ @@ -5672,7 +5674,7 @@ rb_vm_opt_cfunc_p(CALL_CACHE cc, int insn) #define VM_TRACE_HOOK(target_event, val) do { \ if ((pc_events & (target_event)) & enabled_flags) { \ - vm_trace_hook(ec, reg_cfp, pc, pc_events, (target_event), global_hooks, local_hooks, (val)); \ + vm_trace_hook(ec, reg_cfp, pc, pc_events, (target_event), global_hooks, local_hooks_ptr, (val)); \ } \ } while (0) @@ -5688,13 +5690,16 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) } else { const rb_iseq_t *iseq = reg_cfp->iseq; + VALUE iseq_val = (VALUE)iseq; size_t pos = pc - ISEQ_BODY(iseq)->iseq_encoded; rb_event_flag_t pc_events = rb_iseq_event_flags(iseq, pos); rb_hook_list_t *local_hooks = iseq->aux.exec.local_hooks; + rb_hook_list_t *const *local_hooks_ptr = &iseq->aux.exec.local_hooks; rb_event_flag_t iseq_local_events = local_hooks != NULL ? local_hooks->events : 0; rb_hook_list_t *bmethod_local_hooks = NULL; + rb_hook_list_t **bmethod_local_hooks_ptr = NULL; rb_event_flag_t bmethod_local_events = 0; - bool bmethod_frame = VM_FRAME_BMETHOD_P(reg_cfp); + const bool bmethod_frame = VM_FRAME_BMETHOD_P(reg_cfp); enabled_flags |= iseq_local_events; VM_ASSERT((iseq_local_events & ~ISEQ_TRACE_EVENTS) == 0); @@ -5703,6 +5708,7 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) const rb_callable_method_entry_t *me = rb_vm_frame_method_entry(reg_cfp); VM_ASSERT(me->def->type == VM_METHOD_TYPE_BMETHOD); bmethod_local_hooks = me->def->body.bmethod.hooks; + bmethod_local_hooks_ptr = &me->def->body.bmethod.hooks; if (bmethod_local_hooks) { bmethod_local_events = bmethod_local_hooks->events; } @@ -5745,7 +5751,7 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) /* check traces */ if ((pc_events & RUBY_EVENT_B_CALL) && bmethod_frame && (bmethod_events & RUBY_EVENT_CALL)) { /* b_call instruction running as a method. Fire call event. */ - vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_CALL, RUBY_EVENT_CALL, global_hooks, bmethod_local_hooks, Qundef); + vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_CALL, RUBY_EVENT_CALL, global_hooks, bmethod_local_hooks_ptr, Qundef); } VM_TRACE_HOOK(RUBY_EVENT_CLASS | RUBY_EVENT_CALL | RUBY_EVENT_B_CALL, Qundef); VM_TRACE_HOOK(RUBY_EVENT_LINE, Qundef); @@ -5754,8 +5760,15 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) VM_TRACE_HOOK(RUBY_EVENT_END | RUBY_EVENT_RETURN | RUBY_EVENT_B_RETURN, TOPN(0)); if ((pc_events & RUBY_EVENT_B_RETURN) && bmethod_frame && (bmethod_events & RUBY_EVENT_RETURN)) { /* b_return instruction running as a method. Fire return event. */ - vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_RETURN, RUBY_EVENT_RETURN, global_hooks, bmethod_local_hooks, TOPN(0)); + vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_RETURN, RUBY_EVENT_RETURN, global_hooks, bmethod_local_hooks_ptr, TOPN(0)); } + + // Pin the iseq since `local_hooks_ptr` points inside the iseq's slot on the GC heap. + // We need the pointer to stay valid in case compaction happens in a trace hook. + // + // Similar treatment is unnecessary for `bmethod_local_hooks_ptr` since + // storage for `rb_method_definition_t` is not on the GC heap. + RB_GC_GUARD(iseq_val); } } } -- cgit v1.2.3