From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.9 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 67A2B211B3 for ; Mon, 3 Dec 2018 09:35:34 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] process.c: fix ETXTBUSY from MJIT compiler process Date: Mon, 3 Dec 2018 09:35:34 +0000 Message-Id: <20181203093534.4235-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: This affects test/ruby/test_process.rb (test_execopt_env_path). Since MJIT uses vfork+execve in a separate thread, there can be small window in-between vfork and execve where tmp_script.cmd is held open by the vforked child. vfork only pauses the MJIT thread, not any Ruby Threads, so our call to Process.spawn will hit ETXTBUSY in that window unless we fork. main thread | MJIT thread ---------------------------------------------------- fd = open(tmp) | | | vfork for CC | CC running write | | --------------- fchmod | | sees "fd" here close(fd) | | Process.spawn called | | vfork (spawn)| (new process) | | | execve => TXTBUSY | | | | | execve (FD_CLOEXEC on fd) | | vfork returns | Holding the waitpid_lock whenever we intend to spawn a process prevents the MJIT thread from spawning a process while we are spawning in Ruby-land. --- process.c | 29 +++++++++++++++++++++-------- test/ruby/test_process.rb | 5 ----- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/process.c b/process.c index 56a90e770b..ae87093593 100644 --- a/process.c +++ b/process.c @@ -916,6 +916,8 @@ do_waitpid(rb_pid_t pid, int *st, int flags) #endif } +#define WAITPID_LOCK_ONLY ((struct waitpid_state *)-1) + struct waitpid_state { struct list_node wnode; rb_execution_context_t *ec; @@ -3923,12 +3925,13 @@ retry_fork_async_signal_safe(int *status, int *ep, volatile int try_gc = 1; struct child_handler_disabler_state old; int err; + rb_vm_t *vm = w && WAITPID_USE_SIGCHLD ? GET_VM() : 0; while (1) { prefork(); disable_child_handler_before_fork(&old); - if (w && WAITPID_USE_SIGCHLD) { - rb_native_mutex_lock(&rb_ec_vm_ptr(w->ec)->waitpid_lock); + if (vm) { + rb_native_mutex_lock(&vm->waitpid_lock); } #ifdef HAVE_WORKING_VFORK if (!has_privilege()) @@ -3954,12 +3957,12 @@ retry_fork_async_signal_safe(int *status, int *ep, #endif } err = errno; - if (w && WAITPID_USE_SIGCHLD) { - if (pid > 0) { + if (vm) { + if (pid > 0 && w != WAITPID_LOCK_ONLY) { w->pid = pid; - list_add(&rb_ec_vm_ptr(w->ec)->waiting_pids, &w->wnode); + list_add(&vm->waiting_pids, &w->wnode); } - rb_native_mutex_unlock(&rb_ec_vm_ptr(w->ec)->waitpid_lock); + rb_native_mutex_unlock(&vm->waitpid_lock); } disable_child_handler_fork_parent(&old); if (0 < pid) /* fork succeed, parent process */ @@ -3995,7 +3998,8 @@ fork_check_err(int *status, int (*chfunc)(void*, char *, size_t), void *charg, error_occurred = recv_child_error(ep[0], &err, errmsg, errmsg_buflen); if (error_occurred) { if (status) { - VM_ASSERT(w == 0 && "only used by extensions"); + VM_ASSERT((w == 0 || w == WAITPID_LOCK_ONLY) && + "only used by extensions"); rb_protect(proc_syswait, (VALUE)pid, status); } else if (!w) { @@ -4340,7 +4344,7 @@ rb_spawn_process(struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen) rb_last_status_set((status & 0xff) << 8, 0); pid = 1; /* dummy */ # endif - if (eargp->waitpid_state) { + if (eargp->waitpid_state && eargp->waitpid_state != WAITPID_LOCK_ONLY) { eargp->waitpid_state->pid = pid; } rb_execarg_run_options(&sarg, NULL, errmsg, errmsg_buflen); @@ -4369,6 +4373,15 @@ static rb_pid_t rb_execarg_spawn(VALUE execarg_obj, char *errmsg, size_t errmsg_buflen) { struct spawn_args args; + struct rb_execarg *eargp = rb_execarg_get(execarg_obj); + + /* + * Prevent a race with MJIT where the compiler process where + * can hold an FD of ours in between vfork + execve + */ + if (!eargp->waitpid_state && mjit_enabled) { + eargp->waitpid_state = WAITPID_LOCK_ONLY; + } args.execarg = execarg_obj; args.errmsg.ptr = errmsg; diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb index 4c212c13f2..816751ec81 100644 --- a/test/ruby/test_process.rb +++ b/test/ruby/test_process.rb @@ -343,11 +343,6 @@ def test_execopts_env end def test_execopt_env_path - # http://ci.rvm.jp/results/trunk-mjit@silicon-docker/1455223 - # http://ci.rvm.jp/results/trunk-mjit@silicon-docker/1450027 - # http://ci.rvm.jp/results/trunk-mjit@silicon-docker/1469867 - skip 'this randomly fails with MJIT' if RubyVM::MJIT.enabled? - bug8004 = '[ruby-core:53103] [Bug #8004]' Dir.mktmpdir do |d| open("#{d}/tmp_script.cmd", "w") {|f| f.puts ": ;"; f.chmod(0755)} -- EW