ruby-changes:72023
From: Alan <ko1@a...>
Date: Tue, 31 May 2022 02:54:40 +0900 (JST)
Subject: [ruby-changes:72023] a687756284 (master): Fix use-after-free with interacting TracePoints
https://git.ruby-lang.org/ruby.git/commit/?id=a687756284 From a687756284187887835aa345adc89b2718054e4a Mon Sep 17 00:00:00 2001 From: Alan Wu <XrXr@u...> 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] --- test/ruby/test_gc_compact.rb | 17 +++++++++++++++++ test/ruby/test_settracefunc.rb | 14 ++++++++++++++ vm_insnhelper.c | 23 ++++++++++++++++++----- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/test/ruby/test_gc_compact.rb b/test/ruby/test_gc_compact.rb index da0023e6f3..be27199cdc 100644 --- a/test/ruby/test_gc_compact.rb +++ b/test/ruby/test_gc_compact.rb @@ -200,4 +200,21 @@ class TestGCCompact < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_gc_compact.rb#L200 GC.compact assert_equal count + 1, GC.stat(:compact_count) end + + def test_compacting_from_trace_point + obj = Object.new + def obj.tracee + :ret # expected to emit both line and call event from one instruction + end + + results = [] + TracePoint.new(:call, :line) do |tp| + results << tp.event + GC.verify_compaction_references + end.enable(target: obj.method(:tracee)) do + obj.tracee + end + + assert_equal([:call, :line], results) + end end diff --git a/test/ruby/test_settracefunc.rb b/test/ruby/test_settracefunc.rb index b43f9c114d..37c8b37c59 100644 --- a/test/ruby/test_settracefunc.rb +++ b/test/ruby/test_settracefunc.rb @@ -2561,6 +2561,20 @@ CODE https://github.com/ruby/ruby/blob/trunk/test/ruby/test_settracefunc.rb#L2561 end bar EOS + + assert_normal_exit(<<-EOS, 'Bug #18730') + def bar + 42 + end + tp_line = TracePoint.new(:line) do |tp0| + tp_multi1 = TracePoint.new(:return, :b_return, :line) do |tp| + tp0.disable + end + tp_multi1.enable + end + tp_line.enable(target: method(:bar)) + bar + EOS end def test_stat_exists 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 https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5629 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 https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5644 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) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5674 #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) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5690 } 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) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5708 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) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5751 /* 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) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L5760 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.1 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/