Skip to content

Commit

Permalink
Fix origin iclass pointer for modules
Browse files Browse the repository at this point in the history
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 also includes a fix for Module#included_modules to skip
iclasses with origins.

Fixes [Bug #16736]
  • Loading branch information
jeremyevans committed May 22, 2020
1 parent ef13558 commit c745a60
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 12 deletions.
24 changes: 19 additions & 5 deletions class.c
Expand Up @@ -837,7 +837,7 @@ rb_include_class_new(VALUE module, VALUE super)
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;
}
Expand Down Expand Up @@ -925,8 +925,9 @@ clear_module_cache_i(ID id, VALUE val, void *data)
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;

Expand Down Expand Up @@ -979,11 +980,24 @@ include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super)
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)) {
Expand Down Expand Up @@ -1093,7 +1107,7 @@ rb_mod_included_modules(VALUE mod)
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);
Expand Down
17 changes: 11 additions & 6 deletions gc.c
Expand Up @@ -2815,9 +2815,11 @@ obj_free(rb_objspace_t *objspace, VALUE obj)
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));
}
Expand Down Expand Up @@ -3911,7 +3913,8 @@ obj_memsize_of(VALUE obj, int use_all_types)
}
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));
}
Expand Down Expand Up @@ -5432,7 +5435,8 @@ gc_mark_children(rb_objspace_t *objspace, VALUE obj)
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)) {
Expand Down Expand Up @@ -8296,7 +8300,8 @@ gc_update_object_references(rb_objspace_t *objspace, VALUE obj)
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)) {
Expand Down
10 changes: 9 additions & 1 deletion internal/class.h
Expand Up @@ -95,9 +95,10 @@ typedef struct rb_classext_struct rb_classext_t;
#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);
Expand All @@ -120,6 +121,7 @@ VALUE rb_singleton_class_get(VALUE obj);
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);
Expand All @@ -136,6 +138,12 @@ RCLASS_SET_ORIGIN(VALUE klass, VALUE origin)
if (klass != origin) FL_SET(origin, RICLASS_IS_ORIGIN);
}

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)
{
Expand Down
59 changes: 59 additions & 0 deletions test/ruby/test_module.rb
Expand Up @@ -479,6 +479,28 @@ def test_include_with_no_args
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 ||
Expand Down Expand Up @@ -540,6 +562,16 @@ def test_included_modules
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)
Expand Down Expand Up @@ -2043,6 +2075,33 @@ def test_prepend_included_modules
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]'

Expand Down

0 comments on commit c745a60

Please sign in to comment.