Fix iseq rb_hook_list_t leak

[XrXr]

After df31715, there is no free call corresponding to
the rb_hook_list_t allocation in iseq_add_local_tracepoint() anymore. Free it when sweeping.

Patch when I first opened this PR:

diff --git a/iseq.c b/iseq.c
index 9af7b54efff..75513fc24cb 100644
--- a/iseq.c
+++ b/iseq.c
@@ -136,6 +136,7 @@ rb_iseq_free(const rb_iseq_t *iseq)

     if (iseq && ISEQ_EXECUTABLE_P(iseq) && iseq->aux.exec.local_hooks) {
         rb_hook_list_free(iseq->aux.exec.local_hooks);
+        ruby_xfree(iseq->aux.exec.local_hooks);
     }

     RUBY_FREE_LEAVE("iseq");

[ko1]

It doesn't solve leaks on the following code:

loop{
  def foo
    p 1
  end
  TracePoint.new(:line){}.enable(target: method(:foo)) do
  end
  undef :foo
}

How about to use this patch?

diff --git a/iseq.c b/iseq.c
index f0117d94ce..8336f13320 100644
--- a/iseq.c
+++ b/iseq.c
@@ -3393,9 +3393,7 @@ iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval)
         local_events = iseq->aux.exec.local_hooks->events;
 
         if (local_events == 0) {
-            if (iseq->aux.exec.local_hooks->running == 0) {
-                rb_hook_list_free(iseq->aux.exec.local_hooks);
-            }
+            rb_hook_list_free(iseq->aux.exec.local_hooks);
             ((rb_iseq_t *)iseq)->aux.exec.local_hooks = NULL;
         }
 
diff --git a/vm_trace.c b/vm_trace.c
index 66ea7b24b7..e1b1448854 100644
--- a/vm_trace.c
+++ b/vm_trace.c
@@ -70,7 +70,10 @@ void
 rb_hook_list_free(rb_hook_list_t *hooks)
 {
     hooks->need_clean = TRUE;
-    clean_hooks(GET_EC(), hooks);
+
+    if (hooks->running == 0) {
+        clean_hooks(GET_EC(), hooks);
+    }
 }
 
 /* ruby_vm_event_flags management */
@@ -196,6 +199,7 @@ clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list)
 {
     rb_event_hook_t *hook, **nextp = &list->hooks;
     VM_ASSERT(list->need_clean == TRUE);
+    VM_ASSERT(list->running == 0);
 
     list->events = 0;
     list->need_clean = FALSE;
@@ -217,6 +221,7 @@ clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list)
     }
     else {
         /* local events */
+        ruby_xfree(list);
     }
 }
 
@@ -1241,10 +1246,11 @@ disable_local_event_iseq_i(VALUE target, VALUE iseq_p, VALUE tpval)
         rb_hook_list_t *hooks = def->body.bmethod.hooks;
         VM_ASSERT(hooks != NULL);
         rb_hook_list_remove_tracepoint(hooks, tpval);
-        if (hooks->running == 0) {
+
+        if (hooks->events == 0) {
             rb_hook_list_free(def->body.bmethod.hooks);
+            def->body.bmethod.hooks = NULL;
         }
-        def->body.bmethod.hooks = NULL;
     }
     return ST_CONTINUE;
 }

• rb_hook_list_free() frees the local hook list if running == 0.
• if running > 0 it left the hooks with the deleted mark, and the last hook user clear it.

I confirmed the leak on the above ruby code solves.

[ko1]

need more fix... should I make my own PR?

diff --git a/iseq.c b/iseq.c
index e42525cb9f..331a33e401 100644
--- a/iseq.c
+++ b/iseq.c
@@ -3359,6 +3359,7 @@ iseq_add_local_tracepoint(const rb_iseq_t *iseq, rb_event_flag_t turnon_events,
     if (n > 0) {
         if (iseq->aux.exec.local_hooks == NULL) {
             ((rb_iseq_t *)iseq)->aux.exec.local_hooks = RB_ZALLOC(rb_hook_list_t);
+            iseq->aux.exec.local_hooks->is_local = true;
         }
         rb_hook_list_connect_tracepoint((VALUE)iseq, iseq->aux.exec.local_hooks, tpval, target_line);
     }
@@ -3413,9 +3414,7 @@ iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval)
         local_events = iseq->aux.exec.local_hooks->events;
 
         if (local_events == 0) {
-            if (iseq->aux.exec.local_hooks->running == 0) {
-                rb_hook_list_free(iseq->aux.exec.local_hooks);
-            }
+            rb_hook_list_free(iseq->aux.exec.local_hooks);
             ((rb_iseq_t *)iseq)->aux.exec.local_hooks = NULL;
         }
 
diff --git a/vm_core.h b/vm_core.h
index bcb0255a00..2ea9830e5d 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -609,8 +609,9 @@ void rb_objspace_call_finalizer(struct rb_objspace *);
 typedef struct rb_hook_list_struct {
     struct rb_event_hook_struct *hooks;
     rb_event_flag_t events;
-    unsigned int need_clean;
     unsigned int running;
+    bool need_clean;
+    bool is_local;
 } rb_hook_list_t;
 
 
diff --git a/vm_trace.c b/vm_trace.c
index f1c0bb2598..deb02e97e0 100644
--- a/vm_trace.c
+++ b/vm_trace.c
@@ -69,8 +69,11 @@ static void clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list);
 void
 rb_hook_list_free(rb_hook_list_t *hooks)
 {
-    hooks->need_clean = TRUE;
-    clean_hooks(GET_EC(), hooks);
+    hooks->need_clean = true;
+
+    if (hooks->running == 0) {
+        clean_hooks(GET_EC(), hooks);
+    }
 }
 
 /* ruby_vm_event_flags management */
@@ -196,9 +199,10 @@ clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list)
 {
     rb_event_hook_t *hook, **nextp = &list->hooks;
     VM_ASSERT(list->need_clean == TRUE);
+    VM_ASSERT(list->running == 0);
 
     list->events = 0;
-    list->need_clean = FALSE;
+    list->need_clean = false;
 
     while ((hook = *nextp) != 0) {
 	if (hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED) {
@@ -211,19 +215,16 @@ clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list)
 	}
     }
 
-    if (list == rb_ec_ractor_hooks(ec)) {
-        /* global events */
-        update_global_event_hook(list->events);
-    }
-    else {
+    if (list->is_local) {
         /* local events */
+        ruby_xfree(list);
     }
 }
 
 static void
 clean_hooks_check(const rb_execution_context_t *ec, rb_hook_list_t *list)
 {
-    if (UNLIKELY(list->need_clean != FALSE)) {
+    if (UNLIKELY(list->need_clean)) {
         if (list->running == 0) {
             clean_hooks(ec, list);
         }
@@ -246,7 +247,7 @@ remove_event_hook(const rb_execution_context_t *ec, const rb_thread_t *filter_th
 		if (data == Qundef || hook->data == data) {
 		    hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED;
 		    ret+=1;
-		    list->need_clean = TRUE;
+		    list->need_clean = true;
 		}
 	    }
 	}
@@ -1244,10 +1245,11 @@ disable_local_event_iseq_i(VALUE target, VALUE iseq_p, VALUE tpval)
         rb_hook_list_t *hooks = def->body.bmethod.hooks;
         VM_ASSERT(hooks != NULL);
         rb_hook_list_remove_tracepoint(hooks, tpval);
-        if (hooks->running == 0) {
+
+        if (hooks->events == 0) {
             rb_hook_list_free(def->body.bmethod.hooks);
+            def->body.bmethod.hooks = NULL;
         }
-        def->body.bmethod.hooks = NULL;
     }
     return ST_CONTINUE;
 }
@@ -1296,7 +1298,7 @@ rb_hook_list_remove_tracepoint(rb_hook_list_t *list, VALUE tpval)
     while (hook) {
         if (hook->data == tpval) {
             hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED;
-            list->need_clean = TRUE;
+            list->need_clean = true;
         }
         else {
             events |= hook->events;

[XrXr]

@ko1

should I make my own PR?

Yes please. I did a poor job of documenting the leak I found when I wrote this PR and you have a better understanding of the problem. If you don't have time to finish up the PR, though, feel free to ask me to do it.

I'm going to close this PR for now.