From 556d6a5b4586b96c0a468d9d2df78d2b8472331a Mon Sep 17 00:00:00 2001 From: Jakub Zakrzewski Date: Sun, 24 Sep 2017 15:19:18 +0200 Subject: [PATCH] Fix premature GC issues in several constructors. Several constructors would store pointers to newly allocated ruby objects inside an unwrapped C struct on the heap. This, coupled with optimizing compilers, made it possible that these objects were not visible to the GC in the mark phase and prematurely collected, resulting in dangling pointers, crashes due to segfaults, etc. These changes aim to make sure that the objects will be always visible to the GC, by storing pointers to them in local variables protected by RB_GC_GUARD. --- ext/gsl_native/function.c | 18 +++++++------ ext/gsl_native/monte.c | 11 +++++--- ext/gsl_native/multimin.c | 12 ++++++--- ext/gsl_native/multiroots.c | 12 ++++++--- ext/gsl_native/ndlinear.c | 2 +- ext/gsl_native/ntuple.c | 50 ++++++++++++++++++++----------------- ext/gsl_native/ool.c | 1 + 7 files changed, 63 insertions(+), 43 deletions(-) diff --git a/ext/gsl_native/function.c b/ext/gsl_native/function.c index 964e3214..1f96254c 100644 --- a/ext/gsl_native/function.c +++ b/ext/gsl_native/function.c @@ -73,13 +73,16 @@ void gsl_function_mark(gsl_function *f) static VALUE rb_gsl_function_alloc(int argc, VALUE *argv, VALUE klass) { gsl_function *f = NULL; - VALUE obj; + VALUE obj, params; f = ALLOC(gsl_function); f->function = &rb_gsl_function_f; - /* (VALUE) f->params = rb_ary_new2(2);*/ - f->params = (void *) rb_ary_new2(2); - rb_ary_store((VALUE) f->params, 1, Qnil); + + params = rb_ary_new2(2); + rb_ary_store(params, 1, Qnil); + f->params = (void *) params; + obj = Data_Wrap_Struct(klass, gsl_function_mark, gsl_function_free, f); + RB_GC_GUARD(params); rb_gsl_function_set_f(argc, argv, obj); return obj; } @@ -338,19 +341,20 @@ static void gsl_function_fdf_mark(gsl_function_fdf *f); static VALUE rb_gsl_function_fdf_new(int argc, VALUE *argv, VALUE klass) { gsl_function_fdf *F = NULL; - VALUE ary; + VALUE ary, ret; size_t i; F = ALLOC(gsl_function_fdf); F->f = &rb_gsl_function_fdf_f; F->df = &rb_gsl_function_fdf_df; F->fdf = &rb_gsl_function_fdf_fdf; ary = rb_ary_new2(4); - /* (VALUE) F->params = ary;*/ F->params = (void *) ary; rb_ary_store(ary, 2, Qnil); rb_ary_store(ary, 3, Qnil); for (i = 0; (int) i < argc; i++) setfunc(i, argv, F); - return Data_Wrap_Struct(klass, gsl_function_fdf_mark, gsl_function_fdf_free, F); + ret = Data_Wrap_Struct(klass, gsl_function_fdf_mark, gsl_function_fdf_free, F); + RB_GC_GUARD(ary); + return ret; } static void gsl_function_fdf_free(gsl_function_fdf *f) diff --git a/ext/gsl_native/monte.c b/ext/gsl_native/monte.c index 3559001a..309d7638 100644 --- a/ext/gsl_native/monte.c +++ b/ext/gsl_native/monte.c @@ -91,13 +91,16 @@ static void gsl_monte_function_mark(gsl_monte_function *f) static VALUE rb_gsl_monte_function_new(int argc, VALUE *argv, VALUE klass) { gsl_monte_function *f; - VALUE obj; + VALUE obj, params; f = ALLOC(gsl_monte_function); f->f = &rb_gsl_monte_function_f; - /* (VALUE) f->params = rb_ary_new2(2);*/ - f->params = (void *) rb_ary_new2(2); - rb_ary_store((VALUE) f->params, 1, Qnil); + + params = rb_ary_new2(2); + rb_ary_store(params, 1, Qnil); + f->params = (void *) params; + obj = Data_Wrap_Struct(klass, gsl_monte_function_mark, gsl_monte_function_free, f); + RB_GC_GUARD(params); rb_gsl_monte_function_set_f(argc, argv, obj); return obj; } diff --git a/ext/gsl_native/multimin.c b/ext/gsl_native/multimin.c index 60aa966c..803e5c00 100644 --- a/ext/gsl_native/multimin.c +++ b/ext/gsl_native/multimin.c @@ -75,7 +75,7 @@ static void gsl_multimin_function_fdf_mark(gsl_multimin_function_fdf *F) static VALUE rb_gsl_multimin_function_new(int argc, VALUE *argv, VALUE klass) { gsl_multimin_function *F = NULL; - VALUE ary; + VALUE ary, ret; size_t i; F = ALLOC(gsl_multimin_function); F->f = &rb_gsl_multimin_function_f; @@ -98,7 +98,9 @@ static VALUE rb_gsl_multimin_function_new(int argc, VALUE *argv, VALUE klass) default: rb_raise(rb_eArgError, "wrong number of arguments"); } - return Data_Wrap_Struct(klass, gsl_multimin_function_mark, gsl_multimin_function_free, F); + ret = Data_Wrap_Struct(klass, gsl_multimin_function_mark, gsl_multimin_function_free, F); + RB_GC_GUARD(ary); + return ret; } static void gsl_multimin_function_free(gsl_multimin_function *f) @@ -209,7 +211,7 @@ static void set_function_fdf(int argc, VALUE *argv, gsl_multimin_function_fdf *F static VALUE rb_gsl_multimin_function_fdf_new(int argc, VALUE *argv, VALUE klass) { gsl_multimin_function_fdf *F = NULL; - VALUE ary; + VALUE ary, ret; F = ALLOC(gsl_multimin_function_fdf); F->f = &rb_gsl_multimin_function_fdf_f; F->df = &rb_gsl_multimin_function_fdf_df; @@ -220,7 +222,9 @@ static VALUE rb_gsl_multimin_function_fdf_new(int argc, VALUE *argv, VALUE klass rb_ary_store(ary, 2, Qnil); rb_ary_store(ary, 3, Qnil); set_function_fdf(argc, argv, F); - return Data_Wrap_Struct(klass, gsl_multimin_function_fdf_mark, gsl_multimin_function_fdf_free, F); + ret = Data_Wrap_Struct(klass, gsl_multimin_function_fdf_mark, gsl_multimin_function_fdf_free, F); + RB_GC_GUARD(ary); + return ret; } static void gsl_multimin_function_fdf_free(gsl_multimin_function_fdf *f) diff --git a/ext/gsl_native/multiroots.c b/ext/gsl_native/multiroots.c index 45675aa8..f5738354 100644 --- a/ext/gsl_native/multiroots.c +++ b/ext/gsl_native/multiroots.c @@ -62,7 +62,7 @@ static const gsl_multiroot_fdfsolver_type* get_fdfsolver_type(VALUE t); static VALUE rb_gsl_multiroot_function_new(int argc, VALUE *argv, VALUE klass) { gsl_multiroot_function *F = NULL; - VALUE ary; + VALUE ary, ret; size_t i; F = ALLOC(gsl_multiroot_function); F->f = &rb_gsl_multiroot_function_f; @@ -86,7 +86,9 @@ static VALUE rb_gsl_multiroot_function_new(int argc, VALUE *argv, VALUE klass) rb_raise(rb_eArgError, "wrong number of arguments"); break; } - return Data_Wrap_Struct(klass, gsl_multiroot_function_mark, gsl_multiroot_function_free, F); + ret = Data_Wrap_Struct(klass, gsl_multiroot_function_mark, gsl_multiroot_function_free, F); + RB_GC_GUARD(ary); + return ret; } static void gsl_multiroot_function_free(gsl_multiroot_function *f) @@ -212,7 +214,7 @@ static void set_function_fdf(int argc, VALUE *argv, gsl_multiroot_function_fdf * static VALUE rb_gsl_multiroot_function_fdf_new(int argc, VALUE *argv, VALUE klass) { gsl_multiroot_function_fdf *F = NULL; - VALUE ary; + VALUE ary, ret; F = ALLOC(gsl_multiroot_function_fdf); F->f = &rb_gsl_multiroot_function_fdf_f; F->df = &rb_gsl_multiroot_function_fdf_df; @@ -223,7 +225,9 @@ static VALUE rb_gsl_multiroot_function_fdf_new(int argc, VALUE *argv, VALUE klas rb_ary_store(ary, 2, Qnil); rb_ary_store(ary, 3, Qnil); set_function_fdf(argc, argv, F); - return Data_Wrap_Struct(klass, gsl_multiroot_function_fdf_mark, gsl_multiroot_function_fdf_free, F); + ret = Data_Wrap_Struct(klass, gsl_multiroot_function_fdf_mark, gsl_multiroot_function_fdf_free, F); + RB_GC_GUARD(ary); + return ret; } static void gsl_multiroot_function_fdf_free(gsl_multiroot_function_fdf *f) diff --git a/ext/gsl_native/ndlinear.c b/ext/gsl_native/ndlinear.c index 92ee1b22..17d873ca 100644 --- a/ext/gsl_native/ndlinear.c +++ b/ext/gsl_native/ndlinear.c @@ -94,6 +94,7 @@ static VALUE rb_gsl_multifit_ndlinear_alloc(int argc, VALUE *argv, VALUE klass) free((size_t*) N); wspace = Data_Wrap_Struct(cWorkspace, multifit_ndlinear_mark, gsl_multifit_ndlinear_free, w); + RB_GC_GUARD(params); return wspace; } @@ -317,4 +318,3 @@ void Init_ndlinear(VALUE module) } #endif - diff --git a/ext/gsl_native/ntuple.c b/ext/gsl_native/ntuple.c index fb674ff0..9d5e8f6e 100644 --- a/ext/gsl_native/ntuple.c +++ b/ext/gsl_native/ntuple.c @@ -133,21 +133,25 @@ VALUE rb_gsl_ntuple_data(VALUE obj) /***** select_fn *****/ -static gsl_ntuple_select_fn* gsl_ntuple_select_fn_alloc(); static void gsl_ntuple_select_fn_free(gsl_ntuple_select_fn *ptr); int rb_gsl_ntuple_select_fn_f(void *data, void *p); static void gsl_ntuple_select_fn_mark(gsl_ntuple_select_fn *ptr); -static gsl_ntuple_select_fn* gsl_ntuple_select_fn_alloc() +static VALUE gsl_ntuple_select_fn_alloc(VALUE klass) { + VALUE params, ret; gsl_ntuple_select_fn *ptr = NULL; ptr = ALLOC(gsl_ntuple_select_fn); if (ptr == NULL) rb_raise(rb_eRuntimeError, "malloc failed"); ptr->function = &rb_gsl_ntuple_select_fn_f; - /* (VALUE) ptr->params = rb_ary_new2(3);*/ - ptr->params = (void *) rb_ary_new2(3); - rb_ary_store((VALUE) ptr->params, 1, Qnil); - return ptr; + + params = rb_ary_new2(3); + rb_ary_store(params, 1, Qnil); + ptr->params = (void *) params; + + ret = Data_Wrap_Struct(klass, gsl_ntuple_select_fn_mark, gsl_ntuple_select_fn_free, ptr); + RB_GC_GUARD(params); + return ret; } static void gsl_ntuple_select_fn_free(gsl_ntuple_select_fn *ptr) @@ -244,30 +248,32 @@ static VALUE rb_gsl_ntuple_select_fn_set_params(int argc, VALUE *argv, VALUE obj static VALUE rb_gsl_ntuple_select_fn_new(int argc, VALUE *argv, VALUE klass) { - gsl_ntuple_select_fn *F = NULL; - VALUE ff; - F = gsl_ntuple_select_fn_alloc(); - ff = Data_Wrap_Struct(klass, gsl_ntuple_select_fn_mark, gsl_ntuple_select_fn_free, F); - rb_gsl_ntuple_select_fn_set_f(argc, argv, ff); - return ff; + VALUE F; + F = gsl_ntuple_select_fn_alloc(klass); + rb_gsl_ntuple_select_fn_set_f(argc, argv, F); + return F; } /***** value_fn *****/ -static gsl_ntuple_value_fn* gsl_ntuple_value_fn_alloc(); static void gsl_ntuple_value_fn_free(gsl_ntuple_value_fn *ptr); static double rb_gsl_ntuple_value_fn_f(void *data, void *p); static void gsl_ntuple_value_fn_mark(gsl_ntuple_value_fn *ptr); -static gsl_ntuple_value_fn* gsl_ntuple_value_fn_alloc() +static VALUE gsl_ntuple_value_fn_alloc(VALUE klass) { + VALUE params, ret; gsl_ntuple_value_fn *ptr = NULL; ptr = ALLOC(gsl_ntuple_value_fn); if (ptr == NULL) rb_raise(rb_eRuntimeError, "malloc failed"); ptr->function = &rb_gsl_ntuple_value_fn_f; - /* (VALUE) ptr->params = rb_ary_new2(3);*/ - ptr->params = (void *) rb_ary_new2(3); + + params = rb_ary_new2(3); + ptr->params = (void *) params; rb_ary_store((VALUE) ptr->params, 1, Qnil); - return ptr; + + ret = Data_Wrap_Struct(klass, gsl_ntuple_value_fn_mark, gsl_ntuple_value_fn_free, ptr); + RB_GC_GUARD(params); + return ret; } static void gsl_ntuple_value_fn_mark(gsl_ntuple_value_fn *ptr) @@ -362,12 +368,10 @@ static VALUE rb_gsl_ntuple_value_fn_set_params(int argc, VALUE *argv, VALUE obj) static VALUE rb_gsl_ntuple_value_fn_new(int argc, VALUE *argv, VALUE klass) { - gsl_ntuple_value_fn *F = NULL; - VALUE ff; - F = gsl_ntuple_value_fn_alloc(); - ff = Data_Wrap_Struct(klass, gsl_ntuple_value_fn_mark, gsl_ntuple_value_fn_free, F); - rb_gsl_ntuple_value_fn_set_f(argc, argv, ff); - return ff; + VALUE F; + F = gsl_ntuple_value_fn_alloc(klass); + rb_gsl_ntuple_value_fn_set_f(argc, argv, F); + return F; } /* singleton method */ diff --git a/ext/gsl_native/ool.c b/ext/gsl_native/ool.c index ea8ade4b..b67b599d 100644 --- a/ext/gsl_native/ool.c +++ b/ext/gsl_native/ool.c @@ -448,6 +448,7 @@ static VALUE rb_ool_conmin_function_alloc(int argc, VALUE *argv, VALUE klass) rb_ary_store(ary, 4, Qnil); /* params */ // set_functions(argc, argv, F); obj = Data_Wrap_Struct(klass, rb_ool_conmin_function_mark, free, F); + RB_GC_GUARD(ary); rb_ool_conmin_function_set(argc, argv, obj); return obj; }