Bug #14070: Refining a module dumps core

• Status: Closed
• 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: DONTNEED, 2.4: DONE

[mame (Yusuke Endoh)]

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

[shugo (Shugo Maeda)]

• Status changed from Open to Assigned

[shugo (Shugo Maeda)]

• Assignee changed from shugo (Shugo Maeda) to matz (Yukihiro Matsumoto)

mame (Yusuke Endoh) wrote:

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.

The superclass of the refinement module is set for super in a refined method.

module M
  def foo
    "M#foo"
  end
end

class C
  include M
end

module M2
  refine M do
    def foo
      "#{super} M@M2#foo"
    end
  end
end

using M2

C.new.foo

At first I didn't think it works with modules, and meant to prohibit super in a refined method of a module,
but I committed code without noticing this problem. (see #12534)

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.

Without the fix, we have to prohibit super in a refined method of modules, but it's a breaking change.
Is it acceptable, Matz?

[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

[shugo (Shugo Maeda)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r60980.

---

Modules should not have subclasses.

When refining a module, the module was set to the superclass of its refinement,
and a segmentation fault occurred.
The superclass of the refinement should be an iclass of the module.
[ruby-core:83617] [Bug #14070]

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.3: UNKNOWN, 2.4: UNKNOWN to 2.3: DONTNEED, 2.4: REQUIRED

You cannot refine Module on 2.3.x.
I set DONTNEED for 2.3.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.3: DONTNEED, 2.4: REQUIRED to 2.3: DONTNEED, 2.4: DONE

ruby_2_4 r62233 merged revision(s) 60980,60984.