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]

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
     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
     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
 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);
         }
     }
 }