From 5c892da7d7974aeed8e7dd97bb31d2394cc19356 Mon Sep 17 00:00:00 2001
From: Yusuke Endoh <mame@ruby-lang.org>
Date: Tue, 9 Nov 2021 17:06:01 +0900
Subject: [PATCH] class.c: descendants must not cause GC until the result array
 is created

Follow up of 428227472fc6563046d8138aab17f07bef6af753. The previous fix
uses `rb_ary_new_from_values` to create the result array, but it may
trigger the GC.

This second try is to create the result array by `rb_ary_new_capa`
before the second iteration, and assume that `rb_ary_push` does not
trigger GC. This assumption is very fragile, so should be improved in
future.

[Bug #18282] [Feature #14394]
---
 class.c                 | 22 +++++++++++++---------
 test/ruby/test_class.rb |  8 ++++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/class.c b/class.c
index cfdd4ddc9a..b1bd896996 100644
--- a/class.c
+++ b/class.c
@@ -29,6 +29,7 @@
 #include "internal/variable.h"
 #include "ruby/st.h"
 #include "vm_core.h"
+#include "gc.h"
 
 #define id_attached id__attached__
 
@@ -1338,7 +1339,7 @@ rb_mod_ancestors(VALUE mod)
 
 struct subclass_traverse_data
 {
-    VALUE *buffer;
+    VALUE buffer;
     long count;
     long maxcount;
 };
@@ -1349,8 +1350,9 @@ class_descendants_recursive(VALUE klass, VALUE v)
     struct subclass_traverse_data *data = (struct subclass_traverse_data *) v;
 
     if (BUILTIN_TYPE(klass) == T_CLASS && !FL_TEST(klass, FL_SINGLETON)) {
-        if (data->buffer && data->count < data->maxcount) {
-            data->buffer[data->count] = klass;
+        if (data->buffer && data->count < data->maxcount && !rb_objspace_garbage_object_p(klass)) {
+            // assumes that this does not cause GC as long as the length does not exceed the capacity
+            rb_ary_push(data->buffer, klass);
         }
         data->count++;
     }
@@ -1378,24 +1380,26 @@ class_descendants_recursive(VALUE klass, VALUE v)
 VALUE
 rb_class_descendants(VALUE klass)
 {
-    struct subclass_traverse_data data = { NULL, 0, -1 };
+    struct subclass_traverse_data data = { Qfalse, 0, -1 };
 
     // estimate the count of subclasses
     rb_class_foreach_subclass(klass, class_descendants_recursive, (VALUE) &data);
 
     // the following allocation may cause GC which may change the number of subclasses
-    data.buffer = ALLOC_N(VALUE, data.count);
+    data.buffer = rb_ary_new_capa(data.count);
     data.maxcount = data.count;
     data.count = 0;
 
+    size_t gc_count = rb_gc_count();
+
     // enumerate subclasses
     rb_class_foreach_subclass(klass, class_descendants_recursive, (VALUE) &data);
 
-    VALUE ary = rb_ary_new_from_values(data.count, data.buffer);
+    if (gc_count != rb_gc_count()) {
+	rb_bug("GC must not occur during the subclass iteration of Class#descendants");
+    }
 
-    free(data.buffer);
-
-    return ary;
+    return data.buffer;
 }
 
 static void
diff --git a/test/ruby/test_class.rb b/test/ruby/test_class.rb
index 034f4c6d20..4ae230f91e 100644
--- a/test/ruby/test_class.rb
+++ b/test/ruby/test_class.rb
@@ -761,4 +761,12 @@ class TestClass < Test::Unit::TestCase
     100000.times { Class.new(c) }
     assert(c.descendants.size <= 100000)
   end
+
+  def test_descendants_gc_stress
+    10000.times do
+      c = Class.new
+      100.times { Class.new(c) }
+      assert(c.descendants.size <= 100)
+    end
+  end
 end