From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS63949 64.71.152.0/24 X-Spam-Status: No, score=-3.1 required=3.0 tests=AWL,BAYES_00, RCVD_IN_DNSWL_NONE,RDNS_NONE,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from 80x24.org (unknown [64.71.152.64]) by dcvr.yhbt.net (Postfix) with ESMTP id 8B8541F404 for ; Fri, 20 Apr 2018 03:22:10 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] variable.c: fix thread + fork errors in autoload Date: Fri, 20 Apr 2018 03:22:10 +0000 Message-Id: <20180420032210.663-1-e@80x24.org> List-Id: This is fairly non-intrusive bugfix to prevent children from trying to reach into thread stacks of the parent. I will probably reuse this idea and redo r62934, too (same bug). * vm_core.h (typedef struct rb_vm_struct): add fork_gen counter * thread.c (rb_thread_atfork_internal): increment fork_gen * variable.c (struct autoload_data_i): store fork_gen * variable.c (check_autoload_data): remove (replaced with get_...) * variable.c (get_autoload_data): check fork_gen when retrieving * variable.c (check_autoload_required): use get_autoload_data * variable.c (rb_autoloading_value): ditto * variable.c (rb_autoload_p): ditto * variable.c (current_autoload_data): ditto * variable.c (autoload_reset): reset fork_gen, adjust indent * variable.c (rb_autoload_load): set fork_gen when setting state * test/ruby/test_autoload.rb (test_autoload_fork): new test [ruby-core:86410] [Bug #14634] --- test/ruby/test_autoload.rb | 26 ++++++++++++++++++++++++++ thread.c | 1 + variable.c | 32 +++++++++++++++++++++++--------- vm_core.h | 1 + 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb index 0220f3e27d..3095052a81 100644 --- a/test/ruby/test_autoload.rb +++ b/test/ruby/test_autoload.rb @@ -285,6 +285,32 @@ class AutoloadTest end end + def test_autoload_fork + EnvUtil.default_warning do + Tempfile.create(['autoload', '.rb']) {|file| + file.puts 'sleep 0.3; class AutoloadTest; end' + file.close + add_autoload(file.path) + begin + thrs = [] + 3.times do + thrs << Thread.new { AutoloadTest; nil } + thrs << Thread.new { fork { AutoloadTest } } + end + thrs.each(&:join) + thrs.each do |th| + pid = th.value or next + _, status = Process.waitpid2(pid) + assert_predicate status, :success? + end + ensure + remove_autoload_constant + assert_nil $!, '[ruby-core:86410] [Bug #14634]' + end + } + end + end if Process.respond_to?(:fork) + def add_autoload(path) (@autoload_paths ||= []) << path ::Object.class_eval {autoload(:AutoloadTest, path)} diff --git a/thread.c b/thread.c index 1caea4976c..f0ace8326c 100644 --- a/thread.c +++ b/thread.c @@ -4216,6 +4216,7 @@ rb_thread_atfork_internal(rb_thread_t *th, void (*atfork)(rb_thread_t *, const r } rb_vm_living_threads_init(vm); rb_vm_living_threads_insert(vm, th); + vm->fork_gen++; rb_thread_sync_reset_all(); vm->sleeper = 0; diff --git a/variable.c b/variable.c index 46911bff3d..6de7b476e3 100644 --- a/variable.c +++ b/variable.c @@ -21,6 +21,7 @@ #include "ccan/list/list.h" #include "id_table.h" #include "debug_counter.h" +#include "vm_core.h" static struct rb_id_table *rb_global_tbl; static ID autoload, classpath, tmp_classpath, classid; @@ -1857,6 +1858,7 @@ struct autoload_data_i { rb_const_flag_t flag; VALUE value; struct autoload_state *state; /* points to on-stack struct */ + rb_serial_t fork_gen; }; static void @@ -1879,8 +1881,18 @@ static const rb_data_type_t autoload_data_i_type = { 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; -#define check_autoload_data(av) \ - (struct autoload_data_i *)rb_check_typeddata((av), &autoload_data_i_type) +static struct autoload_data_i * +get_autoload_data(VALUE av) +{ + struct autoload_data_i *ele = rb_check_typeddata(av, &autoload_data_i_type); + + /* do not reach across stack for ->state after forking: */ + if (ele && ele->state && ele->fork_gen != GET_VM()->fork_gen) { + ele->state = 0; + ele->fork_gen = 0; + } + return ele; +} RUBY_FUNC_EXPORTED void rb_autoload(VALUE mod, ID id, const char *file) @@ -1980,7 +1992,7 @@ check_autoload_required(VALUE mod, ID id, const char **loadingpath) const char *loading; int safe; - if (!(load = autoload_data(mod, id)) || !(ele = check_autoload_data(load))) { + if (!(load = autoload_data(mod, id)) || !(ele = get_autoload_data(load))) { return 0; } file = ele->feature; @@ -2018,7 +2030,7 @@ rb_autoloading_value(VALUE mod, ID id, VALUE* value, rb_const_flag_t *flag) VALUE load; struct autoload_data_i *ele; - if (!(load = autoload_data(mod, id)) || !(ele = check_autoload_data(load))) { + if (!(load = autoload_data(mod, id)) || !(ele = get_autoload_data(load))) { return 0; } if (ele->state && ele->state->thread == rb_thread_current()) { @@ -2085,8 +2097,9 @@ autoload_reset(VALUE arg) int need_wakeups = 0; if (state->ele->state == state) { - need_wakeups = 1; - state->ele->state = 0; + need_wakeups = 1; + state->ele->state = 0; + state->ele->fork_gen = 0; } /* At the last, move a value defined in autoload to constant table */ @@ -2168,7 +2181,7 @@ rb_autoload_load(VALUE mod, ID id) if (src && loading && strcmp(src, loading) == 0) return Qfalse; /* set ele->state for a marker of autoloading thread */ - if (!(ele = check_autoload_data(load))) { + if (!(ele = get_autoload_data(load))) { return Qfalse; } @@ -2178,6 +2191,7 @@ rb_autoload_load(VALUE mod, ID id) state.thread = rb_thread_current(); if (!ele->state) { ele->state = &state; + ele->fork_gen = GET_VM()->fork_gen; /* * autoload_reset will wake up any threads added to this @@ -2215,7 +2229,7 @@ rb_autoload_p(VALUE mod, ID id) } load = check_autoload_required(mod, id, 0); if (!load) return Qnil; - return (ele = check_autoload_data(load)) ? ele->feature : Qnil; + return (ele = get_autoload_data(load)) ? ele->feature : Qnil; } MJIT_FUNC_EXPORTED void @@ -2638,7 +2652,7 @@ current_autoload_data(VALUE mod, ID id) struct autoload_data_i *ele; VALUE load = autoload_data(mod, id); if (!load) return 0; - ele = check_autoload_data(load); + ele = get_autoload_data(load); if (!ele) return 0; /* for autoloading thread, keep the defined value to autoloading storage */ if (ele->state && (ele->state->thread == rb_thread_current())) { diff --git a/vm_core.h b/vm_core.h index 51b51255b3..5fa11487f7 100644 --- a/vm_core.h +++ b/vm_core.h @@ -539,6 +539,7 @@ typedef struct rb_vm_struct { struct rb_thread_struct *main_thread; struct rb_thread_struct *running_thread; + rb_serial_t fork_gen; struct list_head waiting_fds; /* <=> struct waiting_fd */ struct list_head living_threads; VALUE thgroup_default; -- EW