From e91cf7a1f7da0a2cd580b7be2339b2b1529fbab4 Mon Sep 17 00:00:00 2001 From: kvtb <76634406+kvtb@users.noreply.github.com> Date: Mon, 18 Nov 2024 14:02:21 +0000 Subject: [PATCH 1/2] Fix migration of Hashes Fixes #70 --- src/mrb_thread.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/src/mrb_thread.c b/src/mrb_thread.c index 0c5cdcf..539f10e 100644 --- a/src/mrb_thread.c +++ b/src/mrb_thread.c @@ -25,6 +25,10 @@ #include #include +#if MRUBY_RELEASE_NO > 30100 +# include +#endif + /* For backward compatibility. See also https://github.com/mruby/mruby/commit/79a621dd739faf4cc0958e11d6a887331cf79e48 @@ -389,6 +393,26 @@ struct mrb_time { struct tm datetime; }; + +struct hash_callback_data { + mrb_state *mrb2; + mrb_value nv; +}; + +static int +hash_callback(mrb_state *mrb, mrb_value key, mrb_value val, void *data) { + struct hash_callback_data *hcbd = (struct hash_callback_data *)data; + + int ai = mrb_gc_arena_save(hcbd->mrb2); + + mrb_value k2 = mrb_thread_migrate_value(mrb, key, hcbd->mrb2); + mrb_value v2 = mrb_thread_migrate_value(mrb, val, hcbd->mrb2); + mrb_hash_set(hcbd->mrb2, hcbd->nv, k2, v2); + + mrb_gc_arena_restore(hcbd->mrb2, ai); + return 0; +} + // based on https://gist.github.com/3066997 mrb_value mrb_thread_migrate_value(mrb_state *mrb, mrb_value const v, mrb_state *mrb2) { @@ -460,21 +484,12 @@ mrb_thread_migrate_value(mrb_state *mrb, mrb_value const v, mrb_state *mrb2) { } case MRB_TT_HASH: { - mrb_value ka; - int i, l; - - mrb_value nv = mrb_hash_new(mrb2); - ka = mrb_hash_keys(mrb, v); - l = RARRAY_LEN(ka); - for (i = 0; i < l; i++) { - int ai = mrb_gc_arena_save(mrb2); - mrb_value k = mrb_thread_migrate_value(mrb, mrb_ary_entry(ka, i), mrb2); - mrb_value o = mrb_thread_migrate_value(mrb, mrb_hash_get(mrb, v, k), mrb2); - mrb_hash_set(mrb2, nv, k, o); - mrb_gc_arena_restore(mrb2, ai); - } - migrate_simple_iv(mrb, v, mrb2, nv); - return nv; + struct hash_callback_data hkbd; + hkbd.mrb2 = mrb2; + hkbd.nv = mrb_hash_new(mrb2); + + mrb_hash_foreach(mrb, mrb_hash_ptr(v), hash_callback, &hkbd); + return hkbd.nv; } case MRB_TT_DATA: { @@ -512,6 +527,7 @@ static void* mrb_thread_func(void* data) { mrb_thread_context* context = (mrb_thread_context*) data; mrb_state* mrb = context->mrb; + // todo: handle exceptions, mrb_protect()? context->result = mrb_yield_with_class(mrb, mrb_obj_value(context->proc), context->argc, context->argv, mrb_nil_value(), mrb->object_class); mrb_gc_protect(mrb, context->result); From 9a3b16fcee1c0cd323ffde5ff69b6286eec21888 Mon Sep 17 00:00:00 2001 From: kvtb <76634406+kvtb@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:54:28 +0000 Subject: [PATCH 2/2] Fix memory corruption by assigning `Queue` its own `mrb_state` --- src/mrb_thread.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/mrb_thread.c b/src/mrb_thread.c index 539f10e..a0ffb41 100644 --- a/src/mrb_thread.c +++ b/src/mrb_thread.c @@ -116,6 +116,8 @@ mrb_queue_context_free(mrb_state *mrb, void *p) { pthread_cond_destroy(&context->cond); pthread_mutex_destroy(&context->mutex); pthread_mutex_destroy(&context->queue_lock); + if (context->mrb) + mrb_close(context->mrb); free(p); } } @@ -237,12 +239,9 @@ is_safe_migratable_simple_value(mrb_state *mrb, mrb_value v, mrb_state *mrb2) } break; case MRB_TT_DATA: - if (!is_safe_migratable_datatype(DATA_TYPE(v))) - return FALSE; - break; + return is_safe_migratable_datatype(DATA_TYPE(v)); default: return FALSE; - break; } return TRUE; } @@ -403,13 +402,10 @@ static int hash_callback(mrb_state *mrb, mrb_value key, mrb_value val, void *data) { struct hash_callback_data *hcbd = (struct hash_callback_data *)data; - int ai = mrb_gc_arena_save(hcbd->mrb2); - mrb_value k2 = mrb_thread_migrate_value(mrb, key, hcbd->mrb2); mrb_value v2 = mrb_thread_migrate_value(mrb, val, hcbd->mrb2); mrb_hash_set(hcbd->mrb2, hcbd->nv, k2, v2); - mrb_gc_arena_restore(hcbd->mrb2, ai); return 0; } @@ -478,8 +474,8 @@ mrb_thread_migrate_value(mrb_state *mrb, mrb_value const v, mrb_state *mrb2) { ai = mrb_gc_arena_save(mrb2); for (i=0; imrb_caller = mrb; - mrb2 = mrb_open(); - migrate_all_symbols(mrb, mrb2); - if(!mrb2) { - mrb_raise(mrb, E_RUNTIME_ERROR, "copying mrb_state failed"); + context->mrb = mrb2 = mrb_open(); + if (!mrb2) { + mrb_raise(mrb, E_RUNTIME_ERROR, "mrb_open failed"); } - context->mrb = mrb2; - rproc = mrb_proc_ptr(proc); - context->proc = migrate_rproc(mrb, rproc, mrb2); + migrate_all_symbols(mrb, mrb2); + context->proc = migrate_rproc(mrb, mrb_proc_ptr(proc), mrb2); MRB_PROC_SET_TARGET_CLASS(context->proc, context->mrb->object_class); context->argc = argc; context->argv = calloc(sizeof (mrb_value), context->argc); @@ -736,9 +732,11 @@ mrb_queue_init(mrb_state* mrb, mrb_value self) { check_pthread_error(mrb, pthread_mutex_init(&context->mutex, NULL)); check_pthread_error(mrb, pthread_cond_init(&context->cond, NULL)); check_pthread_error(mrb, pthread_mutex_init(&context->queue_lock, NULL)); - context->mrb = mrb; - context->queue = mrb_ary_new(mrb); - mrb_iv_set(mrb, self, mrb_intern_lit(mrb, "queue"), context->queue); + context->mrb = mrb_open(); + if (!context->mrb) { + mrb_raise(mrb, E_RUNTIME_ERROR, "mrb_open failed"); + } + context->queue = mrb_ary_new(context->mrb); mrb_data_init(self, context, &mrb_queue_context_type); check_pthread_error(mrb, pthread_mutex_lock(&context->queue_lock)); @@ -766,7 +764,7 @@ mrb_queue_clear(mrb_state* mrb, mrb_value self) { mrb_queue_context* context = DATA_PTR(self); mrb_queue_lock(mrb, self); - mrb_ary_clear(mrb, context->queue); + mrb_ary_clear(context->mrb, context->queue); mrb_queue_unlock(mrb, self); return mrb_nil_value(); @@ -842,12 +840,6 @@ mrb_queue_shift(mrb_state* mrb, mrb_value self) { return ret; } -static mrb_value -mrb_queue_num_waiting(mrb_state* mrb, mrb_value self) { - /* TODO: multiple waiting */ - return mrb_fixnum_value(0); -} - static mrb_value mrb_queue_empty_p(mrb_state* mrb, mrb_value self) { mrb_bool ret; @@ -912,7 +904,6 @@ mrb_mruby_thread_gem_init(mrb_state* mrb) { mrb_define_alias(mrb, _class_queue, "deq", "pop"); mrb_define_method(mrb, _class_queue, "shift", mrb_queue_shift, MRB_ARGS_OPT(1)); mrb_define_method(mrb, _class_queue, "size", mrb_queue_size, MRB_ARGS_NONE()); - mrb_define_method(mrb, _class_queue, "num_waiting", mrb_queue_num_waiting, MRB_ARGS_NONE()); mrb_define_method(mrb, _class_queue, "empty?", mrb_queue_empty_p, MRB_ARGS_NONE()); }