[ruby-core:63039] Re: [RFC] vm_method.c (rb_method_entry_make): avoid freed me in m_tbl

Hi,

I don't against this change but I can't understand what is problem and
what is better. It seems moving rb_unlink_method_entry() function to the
end of rb_method_entry_make().

----

BTW, I find another bug.

rb_gc_mark_unlinked_live_method_entries() is located at the end of root
scan, but it should be located at the end of marking, because paused
Fiber can mark another unlinked method entry.

(2014/05/25 2:45), Eric Wong wrote:
> 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;
>  }
> 


-- 
// SASADA Koichi at atdot dot net