[ruby-core:84031] [Ruby trunk Bug#14070] Refining a module dumps core

Issue #14070 has been updated by Eregon (Benoit Daloze).


Can we commit a fix for this?
I now put the spec using `refine module` in quarantine until this is resolved:
https://github.com/ruby/ruby/commit/4d7b0b9112f2adf9e87ef75056f930bf7c1f3dc4#diff-d3d17e341ab819948121f31a45de9bdf

----------------------------------------
Bug #14070: Refining a module dumps core
https://bugs.ruby-lang.org/issues/14070#change-68115

* Author: mame (Yusuke Endoh)
* Status: Assigned
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
* Target version: 2.5
* ruby -v: ruby 2.5.0dev (2017-10-30 trunk 60565) [x86_64-linux]
* Backport: 2.3: UNKNOWN, 2.4: UNKNOWN
----------------------------------------
Including and refining one module at a time, seems to cause "double free" bug.  Here is a short example (ko1 found):

~~~
loop do
  Class.new do
    include Enumerable
  end

  Module.new do
    refine Enumerable do
      def foo
      end
    end
  end
end
~~~

`make test-spec` fails with SEGV very infrequently.

The refinement module (the return value of `refine Enumerable`) seems to be included to Enumerable, and simultaneously, to have Enumerable as a superclass.  Is this intended?  The current VM assumes that a module has no superclass, and that a class is not included to other class.  This discrepancy causes SEGV.

If this is really intended, we need to extend VM to support a module that has a superclass and is included.  The following patch fixes the issue by keeping a included module list in addition to a subclass list.  I'm unsure if this is enough or not.

~~~
diff --git a/class.c b/class.c
index 364f258333..a092fd7cc9 100644
--- a/class.c
+++ b/class.c
@@ -62,14 +62,14 @@ rb_module_add_to_subclasses_list(VALUE module, VALUE iclass)
     entry->klass = iclass;
     entry->next = NULL;
 
-    head = RCLASS_EXT(module)->subclasses;
+    head = RCLASS_EXT(module)->submodules;
     if (head) {
 	entry->next = head;
 	RCLASS_EXT(head->klass)->module_subclasses = &entry->next;
     }
 
-    RCLASS_EXT(module)->subclasses = entry;
-    RCLASS_EXT(iclass)->module_subclasses = &RCLASS_EXT(module)->subclasses;
+    RCLASS_EXT(module)->submodules = entry;
+    RCLASS_EXT(iclass)->module_subclasses = &RCLASS_EXT(module)->submodules;
 }
 
 void
@@ -110,10 +110,8 @@ rb_class_remove_from_module_subclasses(VALUE klass)
 }
 
 void
-rb_class_foreach_subclass(VALUE klass, void (*f)(VALUE, VALUE), VALUE arg)
+rb_class_foreach_subclass(rb_subclass_entry_t *cur, void (*f)(VALUE, VALUE), VALUE arg)
 {
-    rb_subclass_entry_t *cur = RCLASS_EXT(klass)->subclasses;
-
     /* do not be tempted to simplify this loop into a for loop, the order of
        operations is important here if `f` modifies the linked list */
     while (cur) {
@@ -132,7 +130,7 @@ class_detach_subclasses(VALUE klass, VALUE arg)
 void
 rb_class_detach_subclasses(VALUE klass)
 {
-    rb_class_foreach_subclass(klass, class_detach_subclasses, Qnil);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->subclasses, class_detach_subclasses, Qnil);
 }
 
 static void
@@ -144,7 +142,7 @@ class_detach_module_subclasses(VALUE klass, VALUE arg)
 void
 rb_class_detach_module_subclasses(VALUE klass)
 {
-    rb_class_foreach_subclass(klass, class_detach_module_subclasses, Qnil);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->submodules, class_detach_module_subclasses, Qnil);
 }
 
 /**
diff --git a/gc.c b/gc.c
index 0d6163ea98..a85375ffd7 100644
--- a/gc.c
+++ b/gc.c
@@ -2220,14 +2220,12 @@ obj_free(rb_objspace_t *objspace, VALUE obj)
 	if (RCLASS_IV_INDEX_TBL(obj)) {
 	    st_free_table(RCLASS_IV_INDEX_TBL(obj));
 	}
+	if (RCLASS_EXT(obj)->submodules) {
+	    rb_class_detach_module_subclasses(obj);
+	    RCLASS_EXT(obj)->submodules = NULL;
+	}
 	if (RCLASS_EXT(obj)->subclasses) {
-	    if (BUILTIN_TYPE(obj) == T_MODULE) {
-		rb_class_detach_module_subclasses(obj);
-	    }
-	    else {
-		rb_class_detach_subclasses(obj);
-	    }
-	    RCLASS_EXT(obj)->subclasses = NULL;
+	    rb_class_detach_subclasses(obj);
 	}
 	rb_class_remove_from_module_subclasses(obj);
 	rb_class_remove_from_super_subclasses(obj);
diff --git a/internal.h b/internal.h
index ad29434c7c..10b8051748 100644
--- a/internal.h
+++ b/internal.h
@@ -764,6 +764,7 @@ struct rb_classext_struct {
      * in the module's `subclasses` list that indicates that the klass has been
      * included. Hopefully that makes sense.
      */
+    rb_subclass_entry_t *submodules;
     rb_subclass_entry_t **module_subclasses;
     rb_serial_t class_serial;
     const VALUE origin_;
@@ -1077,7 +1078,7 @@ VALUE rb_class_boot(VALUE);
 VALUE rb_class_inherited(VALUE, VALUE);
 VALUE rb_make_metaclass(VALUE, VALUE);
 VALUE rb_include_class_new(VALUE, VALUE);
-void rb_class_foreach_subclass(VALUE klass, void (*f)(VALUE, VALUE), VALUE);
+void rb_class_foreach_subclass(rb_subclass_entry_t *cur, void (*f)(VALUE, VALUE), VALUE);
 void rb_class_detach_subclasses(VALUE);
 void rb_class_detach_module_subclasses(VALUE);
 void rb_class_remove_from_module_subclasses(VALUE);
diff --git a/vm_method.c b/vm_method.c
index 3378f10bd5..817ae09fcb 100644
--- a/vm_method.c
+++ b/vm_method.c
@@ -77,7 +77,8 @@ rb_class_clear_method_cache(VALUE klass, VALUE arg)
 	}
     }
 
-    rb_class_foreach_subclass(klass, rb_class_clear_method_cache, arg);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->subclasses, rb_class_clear_method_cache, arg);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->submodules, rb_class_clear_method_cache, arg);
 }
 
 void
@@ -103,7 +104,14 @@ rb_clear_method_cache_by_class(VALUE klass)
     }
 
     if (klass == rb_mKernel) {
-	rb_subclass_entry_t *entry = RCLASS_EXT(klass)->subclasses;
+	rb_subclass_entry_t *entry = RCLASS_EXT(klass)->submodules;
+
+	for (; entry != NULL; entry = entry->next) {
+	    struct rb_id_table *table = RCLASS_CALLABLE_M_TBL(entry->klass);
+	    if (table)rb_id_table_clear(table);
+	}
+
+	entry = RCLASS_EXT(klass)->subclasses;
 
 	for (; entry != NULL; entry = entry->next) {
 	    struct rb_id_table *table = RCLASS_CALLABLE_M_TBL(entry->klass);
@@ -479,7 +487,8 @@ check_override_opt_method(VALUE klass, VALUE arg)
 	    if (newme != me) rb_vm_check_redefinition_opt_method(me, me->owner);
 	}
     }
-    rb_class_foreach_subclass(klass, check_override_opt_method, (VALUE)mid);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->subclasses, check_override_opt_method, (VALUE)mid);
+    rb_class_foreach_subclass(RCLASS_EXT(klass)->submodules, check_override_opt_method, (VALUE)mid);
 }
 
 /*
~~~



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>