[前][次][番号順一覧][スレッド一覧]

ruby-changes:54696

From: nagachika <ko1@a...>
Date: Wed, 23 Jan 2019 23:14:31 +0900 (JST)
Subject: [ruby-changes:54696] nagachika:r66912 (ruby_2_5): merge revision(s) 62934, 63210, 63215, 63309: [Backport #14634]

nagachika	2019-01-23 23:14:25 +0900 (Wed, 23 Jan 2019)

  New Revision: 66912

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=66912

  Log:
    merge revision(s) 62934,63210,63215,63309: [Backport #14634]
    
    thread_sync.c: avoid reaching across stacks of dead threads
    
    rb_ensure is insufficient cleanup for fork and we must
    reinitialize all waitqueues in the child process.
    
    Unfortunately this increases the footprint of ConditionVariable,
    Queue and SizedQueue by 8 bytes on 32-bit (16 bytes on 64-bit).
    
    [ruby-core:86316] [Bug #14634]
    
    variable.c: fix thread + fork errors in autoload
    
    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]
    
    thread_sync: redo r62934 to use fork_gen
    
    Instead of maintaining linked-lists to store all
    rb_queue/rb_szqueue/rb_condvar structs; store only a fork_gen
    serial number to simplify management of these items.
    
    This reduces initialization costs and avoids the up-front cost
    of resetting all Queue/SizedQueue/ConditionVariable objects at
    fork while saving 8 bytes per-structure on 64-bit.  There are no
    savings on 32-bit.
    
    * thread.c (rb_thread_atfork_internal): remove rb_thread_sync_reset_all call
    * thread_sync.c (rb_thread_sync_reset_all): remove
    * thread_sync.c (queue_live): remove
    * thread_sync.c (queue_free): remove
    * thread_sync.c (struct rb_queue): s/live/fork_gen/
    * thread_sync.c (queue_data_type): use default free
    * thread_sync.c (queue_alloc): remove list_add
    * thread_sync.c (queue_fork_check): new function
    * thread_sync.c (queue_ptr): call queue_fork_check
    * thread_sync.c (szqueue_free): remove
    * thread_sync.c (szqueue_data_type): use default free
    * thread_sync.c (szqueue_alloc): remove list_add
    * thread_sync.c (szqueue_ptr):  check fork_gen via queue_fork_check
    * thread_sync.c (struct rb_condvar): s/live/fork_gen/
    * thread_sync.c (condvar_free): remove
    * thread_sync.c (cv_data_type): use default free
    * thread_sync.c (condvar_ptr): check fork_gen
    * thread_sync.c (condvar_alloc): remove list_add
      [ruby-core:86316] [Bug #14634]
    
    thread_sync.c (condvar_ptr): reset fork_gen after forking
    
    Otherwise the condition variable waiter list will always
    be empty, which is wrong :x
    
    [Bug #14725] [Bug #14634]

  Modified directories:
    branches/ruby_2_5/
  Modified files:
    branches/ruby_2_5/test/ruby/test_autoload.rb
    branches/ruby_2_5/test/thread/test_cv.rb
    branches/ruby_2_5/test/thread/test_queue.rb
    branches/ruby_2_5/thread.c
    branches/ruby_2_5/thread_sync.c
    branches/ruby_2_5/variable.c
    branches/ruby_2_5/version.h
    branches/ruby_2_5/vm_core.h
Index: ruby_2_5/variable.c
===================================================================
--- ruby_2_5/variable.c	(revision 66911)
+++ ruby_2_5/variable.c	(revision 66912)
@@ -20,6 +20,7 @@ https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L20
 #include "ccan/list/list.h"
 #include "id_table.h"
 #include "debug_counter.h"
+#include "vm_core.h"
 
 struct rb_id_table *rb_global_tbl;
 static ID autoload, classpath, tmp_classpath, classid;
@@ -1859,6 +1860,7 @@ struct autoload_data_i { https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L1860
     rb_const_flag_t flag;
     VALUE value;
     struct autoload_state *state; /* points to on-stack struct */
+    rb_serial_t fork_gen;
 };
 
 static void
@@ -1881,8 +1883,18 @@ static const rb_data_type_t autoload_dat https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L1883
     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)
@@ -1982,7 +1994,7 @@ check_autoload_required(VALUE mod, ID id https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L1994
     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;
@@ -2020,7 +2032,7 @@ rb_autoloading_value(VALUE mod, ID id, V https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2032
     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()) {
@@ -2087,8 +2099,9 @@ autoload_reset(VALUE arg) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2099
     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 */
@@ -2170,7 +2183,7 @@ rb_autoload_load(VALUE mod, ID id) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2183
     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;
     }
 
@@ -2180,6 +2193,7 @@ rb_autoload_load(VALUE mod, ID id) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2193
     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
@@ -2217,7 +2231,7 @@ rb_autoload_p(VALUE mod, ID id) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2231
     }
     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;
 }
 
 void
@@ -2646,7 +2660,7 @@ current_autoload_data(VALUE mod, ID id) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/variable.c#L2660
     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())) {
Index: ruby_2_5/test/ruby/test_autoload.rb
===================================================================
--- ruby_2_5/test/ruby/test_autoload.rb	(revision 66911)
+++ ruby_2_5/test/ruby/test_autoload.rb	(revision 66912)
@@ -285,6 +285,32 @@ p Foo::Bar https://github.com/ruby/ruby/blob/trunk/ruby_2_5/test/ruby/test_autoload.rb#L285
     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)}
Index: ruby_2_5/test/thread/test_queue.rb
===================================================================
--- ruby_2_5/test/thread/test_queue.rb	(revision 66911)
+++ ruby_2_5/test/thread/test_queue.rb	(revision 66912)
@@ -565,4 +565,52 @@ class TestQueue < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/ruby_2_5/test/thread/test_queue.rb#L565
       puts 'exit'
     INPUT
   end
+
+  def test_fork_while_queue_waiting
+    q = Queue.new
+    sq = SizedQueue.new(1)
+    thq = Thread.new { q.pop }
+    thsq = Thread.new { sq.pop }
+    Thread.pass until thq.stop? && thsq.stop?
+
+    pid = fork do
+      exit!(1) if q.num_waiting != 0
+      exit!(2) if sq.num_waiting != 0
+      exit!(6) unless q.empty?
+      exit!(7) unless sq.empty?
+      q.push :child_q
+      sq.push :child_sq
+      exit!(3) if q.pop != :child_q
+      exit!(4) if sq.pop != :child_sq
+      exit!(0)
+    end
+    _, s = Process.waitpid2(pid)
+    assert_predicate s, :success?, 'no segfault [ruby-core:86316] [Bug #14634]'
+
+    q.push :thq
+    sq.push :thsq
+    assert_equal :thq, thq.value
+    assert_equal :thsq, thsq.value
+
+    sq.push(1)
+    th = Thread.new { q.pop; sq.pop }
+    thsq = Thread.new { sq.push(2) }
+    Thread.pass until th.stop? && thsq.stop?
+    pid = fork do
+      exit!(1) if q.num_waiting != 0
+      exit!(2) if sq.num_waiting != 0
+      exit!(3) unless q.empty?
+      exit!(4) if sq.empty?
+      exit!(5) if sq.pop != 1
+      exit!(0)
+    end
+    _, s = Process.waitpid2(pid)
+    assert_predicate s, :success?, 'no segfault [ruby-core:86316] [Bug #14634]'
+
+    assert_predicate thsq, :stop?
+    assert_equal 1, sq.pop
+    assert_same sq, thsq.value
+    q.push('restart th')
+    assert_equal 2, th.value
+  end if Process.respond_to?(:fork)
 end
Index: ruby_2_5/test/thread/test_cv.rb
===================================================================
--- ruby_2_5/test/thread/test_cv.rb	(revision 66911)
+++ ruby_2_5/test/thread/test_cv.rb	(revision 66912)
@@ -217,4 +217,23 @@ INPUT https://github.com/ruby/ruby/blob/trunk/ruby_2_5/test/thread/test_cv.rb#L217
       Marshal.dump(condvar)
     end
   end
+
+  def test_condvar_fork
+    mutex = Mutex.new
+    condvar = ConditionVariable.new
+    thrs = (1..10).map do
+      Thread.new { mutex.synchronize { condvar.wait(mutex) } }
+    end
+    thrs.each { 3.times { Thread.pass } }
+    pid = fork do
+      mutex.synchronize { condvar.broadcast }
+      exit!(0)
+    end
+    _, s = Process.waitpid2(pid)
+    assert_predicate s, :success?, 'no segfault [ruby-core:86316] [Bug #14634]'
+    until thrs.empty?
+      mutex.synchronize { condvar.broadcast }
+      thrs.delete_if { |t| t.join(0.01) }
+    end
+  end if Process.respond_to?(:fork)
 end
Index: ruby_2_5/version.h
===================================================================
--- ruby_2_5/version.h	(revision 66911)
+++ ruby_2_5/version.h	(revision 66912)
@@ -1,10 +1,10 @@ https://github.com/ruby/ruby/blob/trunk/ruby_2_5/version.h#L1
 #define RUBY_VERSION "2.5.4"
-#define RUBY_RELEASE_DATE "2019-01-20"
-#define RUBY_PATCHLEVEL 137
+#define RUBY_RELEASE_DATE "2019-01-23"
+#define RUBY_PATCHLEVEL 138
 
 #define RUBY_RELEASE_YEAR 2019
 #define RUBY_RELEASE_MONTH 1
-#define RUBY_RELEASE_DAY 20
+#define RUBY_RELEASE_DAY 23
 
 #include "ruby/version.h"
 
Index: ruby_2_5/thread_sync.c
===================================================================
--- ruby_2_5/thread_sync.c	(revision 66911)
+++ ruby_2_5/thread_sync.c	(revision 66912)
@@ -4,6 +4,14 @@ https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L4
 static VALUE rb_cMutex, rb_cQueue, rb_cSizedQueue, rb_cConditionVariable;
 static VALUE rb_eClosedQueueError;
 
+/*
+ * keep these globally so we can walk and reinitialize them at fork
+ * in the child process
+ */
+static LIST_HEAD(szqueue_list);
+static LIST_HEAD(queue_list);
+static LIST_HEAD(condvar_list);
+
 /* sync_waiter is always on-stack */
 struct sync_waiter {
     rb_thread_t *th;
@@ -556,6 +564,7 @@ void rb_mutex_allow_trap(VALUE self, int https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L564
 #define queue_waitq(q) UNALIGNED_MEMBER_PTR(q, waitq)
 PACKED_STRUCT_UNALIGNED(struct rb_queue {
     struct list_head waitq;
+    rb_serial_t fork_gen;
     const VALUE que;
     int num_waiting;
 });
@@ -601,12 +610,29 @@ queue_alloc(VALUE klass) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L610
     return obj;
 }
 
+static int
+queue_fork_check(struct rb_queue *q)
+{
+    rb_serial_t fork_gen = GET_VM()->fork_gen;
+
+    if (q->fork_gen == fork_gen) {
+        return 0;
+    }
+    /* forked children can't reach into parent thread stacks */
+    q->fork_gen = fork_gen;
+    list_head_init(queue_waitq(q));
+    q->num_waiting = 0;
+    return 1;
+}
+
 static struct rb_queue *
 queue_ptr(VALUE obj)
 {
     struct rb_queue *q;
 
     TypedData_Get_Struct(obj, struct rb_queue, &queue_data_type, q);
+    queue_fork_check(q);
+
     return q;
 }
 
@@ -649,6 +675,11 @@ szqueue_ptr(VALUE obj) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L675
     struct rb_szqueue *sq;
 
     TypedData_Get_Struct(obj, struct rb_szqueue, &szqueue_data_type, sq);
+    if (queue_fork_check(&sq->q)) {
+        list_head_init(szqueue_pushq(sq));
+        sq->num_waiting_push = 0;
+    }
+
     return sq;
 }
 
@@ -885,7 +916,7 @@ queue_do_pop(VALUE self, struct rb_queue https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L916
 	    list_add_tail(&qw.as.q->waitq, &qw.w.node);
 	    qw.as.q->num_waiting++;
 
-	    rb_ensure(queue_sleep, Qfalse, queue_sleep_done, (VALUE)&qw);
+	    rb_ensure(queue_sleep, self, queue_sleep_done, (VALUE)&qw);
 	}
     }
 
@@ -1127,7 +1158,7 @@ rb_szqueue_push(int argc, VALUE *argv, V https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L1158
 	    list_add_tail(pushq, &qw.w.node);
 	    sq->num_waiting_push++;
 
-	    rb_ensure(queue_sleep, Qfalse, szqueue_sleep_done, (VALUE)&qw);
+	    rb_ensure(queue_sleep, self, szqueue_sleep_done, (VALUE)&qw);
 	}
     }
 
@@ -1228,9 +1259,9 @@ rb_szqueue_empty_p(VALUE self) https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L1259
 
 
 /* ConditionalVariable */
-/* TODO: maybe this can be IMEMO */
 struct rb_condvar {
     struct list_head waitq;
+    rb_serial_t fork_gen;
 };
 
 /*
@@ -1277,9 +1308,15 @@ static struct rb_condvar * https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread_sync.c#L1308
 condvar_ptr(VALUE self)
 {
     struct rb_condvar *cv;
+    rb_serial_t fork_gen = GET_VM()->fork_gen;
 
     TypedData_Get_Struct(self, struct rb_condvar, &cv_data_type, cv);
 
+    /* forked children can't reach into parent thread stacks */
+    if (cv->fork_gen != fork_gen) {
+        list_head_init(&cv->waitq);
+    }
+
     return cv;
 }
 
Index: ruby_2_5/thread.c
===================================================================
--- ruby_2_5/thread.c	(revision 66911)
+++ ruby_2_5/thread.c	(revision 66912)
@@ -4217,6 +4217,8 @@ rb_thread_atfork_internal(rb_thread_t *t https://github.com/ruby/ruby/blob/trunk/ruby_2_5/thread.c#L4217
     }
     rb_vm_living_threads_init(vm);
     rb_vm_living_threads_insert(vm, th);
+    vm->fork_gen++;
+
     vm->sleeper = 0;
     clear_coverage();
 }
Index: ruby_2_5/vm_core.h
===================================================================
--- ruby_2_5/vm_core.h	(revision 66911)
+++ ruby_2_5/vm_core.h	(revision 66912)
@@ -507,6 +507,7 @@ typedef struct rb_vm_struct { https://github.com/ruby/ruby/blob/trunk/ruby_2_5/vm_core.h#L507
     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;
     size_t living_thread_num;
Index: ruby_2_5
===================================================================
--- ruby_2_5	(revision 66911)
+++ ruby_2_5	(revision 66912)

Property changes on: ruby_2_5
___________________________________________________________________
Modified: svn:mergeinfo
## -0,0 +0,1 ##
   Merged /trunk:r62934,63210,63215

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/

[前][次][番号順一覧][スレッド一覧]