From: Eric Wong <normalperson@...>
Date: 2014-05-24T17:45:17+00:00
Subject: [ruby-core:62727] [RFC] vm_method.c (rb_method_entry_make): avoid	freed me in m_tbl

rb_unlink_method_entry may cause old_me to be swept before the new
method is inserted; allowing for a window where the method entry may
be prematurely freed/swept but still referenced in the old slot of
the method entry table.

From what I can tell, this is not currently a problem with when using
st.  However, with my proposed ihash method table implementation, this
caused a segfault with the test[1] for bug #8100 where the method entry
is itself a linked list node, there is no st_table_entry.

Right now, this patch will make us more robust against future changes.

[1] test/ruby/test_method.rb:
  test_unlinked_method_entry_in_method_object_bug
---
 Thoughts?  I will likely commit this separately since changing the
 method table implementation may not happen soon.

 vm_method.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/vm_method.c b/vm_method.c
index d551332..beb9008 100644
--- a/vm_method.c
+++ b/vm_method.c
@@ -255,6 +255,7 @@ rb_method_entry_make(VALUE klass, ID mid, rb_method_type_t type,
     st_table *mtbl;
     st_data_t data;
     int make_refined = 0;
+    rb_method_entry_t *old_me;
 
     if (NIL_P(klass)) {
 	klass = rb_cObject;
@@ -279,8 +280,7 @@ rb_method_entry_make(VALUE klass, ID mid, rb_method_type_t type,
 	rb_add_refined_method_entry(refined_class, mid);
     }
     if (type == VM_METHOD_TYPE_REFINED) {
-	rb_method_entry_t *old_me =
-	    lookup_method_table(RCLASS_ORIGIN(klass), mid);
+	old_me = lookup_method_table(RCLASS_ORIGIN(klass), mid);
 	if (old_me) rb_vm_check_redefinition_opt_method(old_me, klass);
     }
     else {
@@ -289,9 +289,12 @@ rb_method_entry_make(VALUE klass, ID mid, rb_method_type_t type,
     mtbl = RCLASS_M_TBL(klass);
 
     /* check re-definition */
+    old_me = 0;
     if (st_lookup(mtbl, mid, &data)) {
-	rb_method_entry_t *old_me = (rb_method_entry_t *)data;
-	rb_method_definition_t *old_def = old_me->def;
+	rb_method_definition_t *old_def;
+
+	old_me = (rb_method_entry_t *)data;
+	old_def = old_me->def;
 
 	if (rb_method_definition_eq(old_def, def)) return old_me;
 #if NOEX_NOREDEF
@@ -329,8 +332,6 @@ rb_method_entry_make(VALUE klass, ID mid, rb_method_type_t type,
 				   rb_id2name(old_def->original_id));
 	    }
 	}
-
-	rb_unlink_method_entry(old_me);
     }
 
     mid = SYM2ID(ID2SYM(mid));
@@ -379,6 +380,9 @@ rb_method_entry_make(VALUE klass, ID mid, rb_method_type_t type,
     }
 
     st_insert(mtbl, mid, (st_data_t) me);
+    if (old_me) {
+	rb_unlink_method_entry(old_me);
+    }
 
     return me;
 }
-- 
Eric Wong