https://git.ruby-lang.org/ruby.git/commit/?id=ad729a1d11

From ad729a1d11c6c57efd2e92803b4e937db0f75252 Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@j...>
Date: Sun, 5 Apr 2020 12:10:42 -0700
Subject: Fix origin iclass pointer for modules

If a module has an origin, and that module is included in another
module or class, previously the iclass created for the module had
an origin pointer to the module's origin instead of the iclass's
origin.

Setting the origin pointer correctly requires using a stack, since
the origin iclass is not created until after the iclass itself.
Use a hidden ruby array to implement that stack.

Correctly assigning the origin pointers in the iclass caused a
use-after-free in GC.  If a module with an origin is included
in a class, the iclass shares a method table with the module
and the iclass origin shares a method table with module origin.

Mark iclass origin with a flag that notes that even though the
iclass is an origin, it shares a method table, so the method table
should not be garbage collected.  The shared method table will be
garbage collected when the module origin is garbage collected.
I've tested that this does not introduce a memory leak.

This change caused a VM assertion failure, which was traced to callable
method entries using the incorrect defined_class.  Update
rb_vm_check_redefinition_opt_method and find_defined_class_by_owner
to treat iclass origins different than class origins to avoid this
issue.

This also includes a fix for Module#included_modules to skip
iclasses with origins.

Fixes [Bug #16736]

diff --git a/class.c b/class.c
index 5270f41..3cc9c59 100644
--- a/class.c
+++ b/class.c
@@ -837,7 +837,7 @@ rb_include_class_new(VALUE module, VALUE super) https://github.com/ruby/ruby/blob/trunk/class.c#L837
     RCLASS_M_TBL(OBJ_WB_UNPROTECT(klass)) =
       RCLASS_M_TBL(OBJ_WB_UNPROTECT(module)); /* TODO: unprotected? */
 
-    RCLASS_SET_ORIGIN(klass, module == RCLASS_ORIGIN(module) ? klass : RCLASS_ORIGIN(module));
+    RCLASS_SET_ORIGIN(klass, klass);
     if (BUILTIN_TYPE(module) == T_ICLASS) {
 	module = RBASIC(module)->klass;
     }
@@ -925,8 +925,9 @@ clear_module_cache_i(ID id, VALUE val, void *data) https://github.com/ruby/ruby/blob/trunk/class.c#L925
 static int
 include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super)
 {
-    VALUE p, iclass;
-    int method_changed = 0, constant_changed = 0;
+    VALUE p, iclass, origin_stack = 0;
+    int method_changed = 0, constant_changed = 0, add_subclass;
+    long origin_len;
     struct rb_id_table *const klass_m_tbl = RCLASS_M_TBL(RCLASS_ORIGIN(klass));
     VALUE original_klass = klass;
 
@@ -979,11 +980,24 @@ include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super) https://github.com/ruby/ruby/blob/trunk/class.c#L980
 	iclass = rb_include_class_new(module, super_class);
 	c = RCLASS_SET_SUPER(c, iclass);
         RCLASS_SET_INCLUDER(iclass, klass);
+        add_subclass = TRUE;
+        if (module != RCLASS_ORIGIN(module)) {
+            if (!origin_stack) origin_stack = rb_ary_tmp_new(2);
+            VALUE origin[2] = {iclass, RCLASS_ORIGIN(module)};
+            rb_ary_cat(origin_stack, origin, 2);
+        }
+        else if (origin_stack && (origin_len = RARRAY_LEN(origin_stack)) > 1 &&
+                 RARRAY_AREF(origin_stack, origin_len - 1) == module) {
+            RCLASS_SET_ORIGIN(RARRAY_AREF(origin_stack, (origin_len -= 2)), iclass);
+            RICLASS_SET_ORIGIN_SHARED_MTBL(iclass);
+            rb_ary_resize(origin_stack, origin_len);
+            add_subclass = FALSE;
+        }
 
 	{
 	    VALUE m = module;
             if (BUILTIN_TYPE(m) == T_ICLASS) m = RBASIC(m)->klass;
-            rb_module_add_to_subclasses_list(m, iclass);
+            if (add_subclass) rb_module_add_to_subclasses_list(m, iclass);
 	}
 
 	if (FL_TEST(klass, RMODULE_IS_REFINEMENT)) {
@@ -1093,7 +1107,7 @@ rb_mod_included_modules(VALUE mod) https://github.com/ruby/ruby/blob/trunk/class.c#L1107
     VALUE origin = RCLASS_ORIGIN(mod);
 
     for (p = RCLASS_SUPER(mod); p; p = RCLASS_SUPER(p)) {
-	if (p != origin && BUILTIN_TYPE(p) == T_ICLASS) {
+        if (p != origin && RCLASS_ORIGIN(p) == p && BUILTIN_TYPE(p) == T_ICLASS) {
 	    VALUE m = RBASIC(p)->klass;
 	    if (RB_TYPE_P(m, T_MODULE))
 		rb_ary_push(ary, m);
diff --git a/gc.c b/gc.c
index b0fa670..9674d67 100644
--- a/gc.c
+++ b/gc.c
@@ -2815,9 +2815,11 @@ obj_free(rb_objspace_t *objspace, VALUE obj) https://github.com/ruby/ruby/blob/trunk/gc.c#L2815
 	break;
       case T_ICLASS:
 	/* Basically , T_ICLASS shares table with the module */
-	if (FL_TEST(obj, RICLASS_IS_ORIGIN)) {
-	    rb_id_table_free(RCLASS_M_TBL(obj));
-	}
+        if (FL_TEST(obj, RICLASS_IS_ORIGIN) &&
+                !FL_TEST(obj, RICLASS_ORIGIN_SHARED_MTBL)) {
+            /* Method table is not shared for origin iclasses of classes */
+            rb_id_table_free(RCLASS_M_TBL(obj));
+        }
 	if (RCLASS_CALLABLE_M_TBL(obj) != NULL) {
 	    rb_id_table_free(RCLASS_CALLABLE_M_TBL(obj));
 	}
@@ -3911,7 +3913,8 @@ obj_memsize_of(VALUE obj, int use_all_types) https://github.com/ruby/ruby/blob/trunk/gc.c#L3913
 	}
 	break;
       case T_ICLASS:
-	if (FL_TEST(obj, RICLASS_IS_ORIGIN)) {
+        if (FL_TEST(obj, RICLASS_IS_ORIGIN) &&
+                !FL_TEST(obj, RICLASS_ORIGIN_SHARED_MTBL)) {
 	    if (RCLASS_M_TBL(obj)) {
 		size += rb_id_table_memsize(RCLASS_M_TBL(obj));
 	    }
@@ -5432,7 +5435,8 @@ gc_mark_children(rb_objspace_t *objspace, VALUE obj) https://github.com/ruby/ruby/blob/trunk/gc.c#L5435
 	break;
 
       case T_ICLASS:
-	if (FL_TEST(obj, RICLASS_IS_ORIGIN)) {
+        if (FL_TEST(obj, RICLASS_IS_ORIGIN) &&
+                !FL_TEST(obj, RICLASS_ORIGIN_SHARED_MTBL)) {
 	    mark_m_tbl(objspace, RCLASS_M_TBL(obj));
 	}
         if (RCLASS_SUPER(obj)) {
@@ -8296,7 +8300,8 @@ gc_update_object_references(rb_objspace_t *objspace, VALUE obj) https://github.com/ruby/ruby/blob/trunk/gc.c#L8300
         break;
 
       case T_ICLASS:
-        if (FL_TEST(obj, RICLASS_IS_ORIGIN)) {
+        if (FL_TEST(obj, RICLASS_IS_ORIGIN) &&
+                !FL_TEST(obj, RICLASS_ORIGIN_SHARED_MTBL)) {
             update_m_tbl(objspace, RCLASS_M_TBL(obj));
         }
         if (RCLASS_SUPER((VALUE)obj)) {
diff --git a/internal/class.h b/internal/class.h
index 4bebb22..7724fe4 100644
--- a/internal/class.h
+++ b/internal/class.h
@@ -95,9 +95,10 @@ typedef struct rb_classext_struct rb_classext_t; https://github.com/ruby/ruby/blob/trunk/internal/class.h#L95
 #endif
 #define RCLASS_INCLUDER(c) (RCLASS_EXT(c)->includer)
 
-#define RCLASS_CLONED     FL_USER6
 #define RICLASS_IS_ORIGIN FL_USER5
+#define RCLASS_CLONED     FL_USER6
 #define RCLASS_REFINED_BY_ANY FL_USER7
+#define RICLASS_ORIGIN_SHARED_MTBL FL_USER8
 
 /* class.c */
 void rb_class_subclass_add(VALUE super, VALUE klass);
@@ -120,6 +121,7 @@ VALUE rb_singleton_class_get(VALUE obj); https://github.com/ruby/ruby/blob/trunk/internal/class.h#L121
 int rb_class_has_methods(VALUE c);
 void rb_undef_methods_from(VALUE klass, VALUE super);
 static inline void RCLASS_SET_ORIGIN(VALUE klass, VALUE origin);
+static inline void RICLASS_SET_ORIGIN_SHARED_MTBL(VALUE iclass);
 static inline VALUE RCLASS_SUPER(VALUE klass);
 static inline VALUE RCLASS_SET_SUPER(VALUE klass, VALUE super);
 static inline void RCLASS_SET_INCLUDER(VALUE iclass, VALUE klass);
@@ -137,6 +139,12 @@ RCLASS_SET_ORIGIN(VALUE klass, VALUE origin) https://github.com/ruby/ruby/blob/trunk/internal/class.h#L139
 }
 
 static inline void
+RICLASS_SET_ORIGIN_SHARED_MTBL(VALUE iclass)
+{
+    FL_SET(iclass, RICLASS_ORIGIN_SHARED_MTBL);
+}
+
+static inline void
 RCLASS_SET_INCLUDER(VALUE iclass, VALUE klass)
 {
     RB_OBJ_WRITE(iclass, &RCLASS_INCLUDER(iclass), klass);
diff --git a/test/ruby/test_module.rb b/test/ruby/test_module.rb
index e3cd627..561f99f 100644
--- a/test/ruby/test_module.rb
+++ b/test/ruby/test_module.rb
@@ -479,6 +479,28 @@ class TestModule < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_module.rb#L479
     assert_raise(ArgumentError) { Module.new { include } }
   end
 
+  def test_gc_prepend_chain
+    assert_separately([], <<-EOS)
+      10000.times { |i|
+        m1 = Module.new do
+          def foo; end
+        end
+        m2 = Module.new do
+          prepend m1
+          def bar; end
+        end
+        m3 = Module.new do
+          def baz; end
+          prepend m2
+        end
+        Class.new do
+          prepend m3
+        end
+      }
+      GC.start
+    EOS
+  end
+
   def test_include_into_module_already_included
     c = Class.new{def foo; [:c] end}
     modules = lambda do ||
@@ -540,6 +562,16 @@ class TestModule < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_module.rb#L562
     assert_equal([Comparable, Kernel], String.included_modules - mixins)
   end
 
+  def test_included_modules_with_prepend
+    m1 = Module.new
+    m2 = Module.new
+    m3 = Module.new
+
+    m2.prepend m1
+    m3.include m2
+    assert_equal([m1, m2], m3.included_modules)
+  end
+
   def test_instance_methods
     assert_equal([:user, :user2], User.instance_methods(false).sort)
     assert_equal([:user, :user2, :mixin].sort, User.instance_methods(true).sort)
@@ -2043,6 +2075,33 @@ class TestModule < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_module.rb#L2075
     assert_include(im, mixin, bug8025)
   end
 
+  def test_prepended_module_with_super_and_alias
+    bug16736 = '[Bug #16736]'
+
+    a = labeled_class("A") do
+      def m; "A"; end
+    end
+    m = labeled_module("M") do
+      prepend Module.new
+
+      def self.included(base)
+        base.alias_method :base_m, :m
+      end
+
+      def m
+        super + "M"
+      end
+
+      def m2
+        base_m
+      end
+    end
+    b = labeled_class("B", a) do
+      include m
+    end
+    assert_equal("AM", b.new.m2, bug16736)
+  end
+
   def test_prepend_super_in_alias
     bug7842 = '[Bug #7842]'
 
diff --git a/ (... truncated)

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/