Allow waitpid(-1) to be woken if a waitpid(pid) call is pending

If two threads are running, with one calling waitpid(-1), and another
calling waitpid($some_pid), and then $some_other_pid exits, we would
expect the waitpid(-1) call to retrieve that exit status; however, it
cannot actually do so until $some_pid _also_ exits.

This patch fixes the issue by unconditionally checking for pending
process group waits on SIGCHLD, and then allowing pending pid-only waits
to "steal" the notification.

[Fixes #19387]

2 files changed, 70 insertions, 5 deletions

diff --git a/process.c b/process.c
index 399f0d7532..97fa336b56 100644
--- a/process.c
+++ b/process.c
@@ -1120,7 +1120,7 @@ rb_sigwait_fd_migrate(rb_vm_t *vm)
 extern volatile unsigned int ruby_nocldwait; /* signal.c */
 /* called by timer thread or thread which acquired sigwait_fd */
 static void
-waitpid_each(struct list_head *head)
+waitpid_each(rb_vm_t *vm, struct list_head *head)
 {
     struct waitpid_state *w = 0, *next;
 
@@ -1130,6 +1130,18 @@ waitpid_each(struct list_head *head)
         if (!ret) continue;
         if (ret == -1) w->errnum = errno;
 
+        if (w->pid <= 0) {
+            /* when waiting for a group of processes, make sure a waiter for a
+             * specific pid is given that event in preference */
+            struct waitpid_state *w_inner = 0, *next_inner;
+            list_for_each_safe(&vm->waiting_pids, w_inner, next_inner, wnode) {
+                if (w_inner->pid == ret) {
+                    /* signal this one instead */
+                    w = w_inner;
+                }
+            }
+        }
+
         w->ret = ret;
         list_del_init(&w->wnode);
         waitpid_signal(w);
@@ -1144,10 +1156,8 @@ ruby_waitpid_all(rb_vm_t *vm)
 {
 #if RUBY_SIGCHLD
     rb_native_mutex_lock(&vm->waitpid_lock);
-    waitpid_each(&vm->waiting_pids);
-    if (list_empty(&vm->waiting_pids)) {
-        waitpid_each(&vm->waiting_grps);
-    }
+    waitpid_each(vm, &vm->waiting_pids);
+    waitpid_each(vm, &vm->waiting_grps);
     /* emulate SA_NOCLDWAIT */
     if (list_empty(&vm->waiting_pids) && list_empty(&vm->waiting_grps)) {
         while (ruby_nocldwait && do_waitpid(-1, 0, WNOHANG) > 0)
diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb
index 91d554ea32..a6de1c8e6d 100644
--- a/test/ruby/test_process.rb
+++ b/test/ruby/test_process.rb
@@ -2637,4 +2637,59 @@ EOS
       end
     end;
   end if Process.respond_to?(:_fork)
+
+  def test_concurrent_group_and_pid_wait
+    # Use a pair of pipes that will make long_pid exit when this test exits, to avoid
+    # leaking temp processes.
+    long_rpipe, long_wpipe = IO.pipe
+    short_rpipe, short_wpipe = IO.pipe
+    # This process should run forever
+    long_pid = fork do
+      [short_rpipe, short_wpipe, long_wpipe].each(&:close)
+      long_rpipe.read
+    end
+    # This process will exit
+    short_pid = fork do
+      [long_rpipe, long_wpipe, short_wpipe].each(&:close)
+      short_rpipe.read
+    end
+    t1, t2, t3 = nil
+    EnvUtil.timeout(5) do
+      t1 = Thread.new do
+        Process.waitpid long_pid
+      end
+      # Wait for us to be blocking in a call to waitpid2
+      Thread.pass until t1.stop?
+      short_wpipe.close # Make short_pid exit
+
+      # The short pid has exited, so -1 should pick that up.
+      assert_equal short_pid, Process.waitpid(-1)
+
+      # Terminate t1 for the next phase of the test.
+      t1.kill
+      t1.join
+
+      t2 = Thread.new do
+        Process.waitpid -1
+      rescue Errno::ECHILD
+        nil
+      end
+      Thread.pass until t2.stop?
+      t3 = Thread.new do
+        Process.waitpid long_pid
+      rescue Errno::ECHILD
+        nil
+      end
+      Thread.pass until t3.stop?
+
+      # it's actually nondeterministic which of t2 or t3 will receive the wait (this
+      # nondeterminism comes from the behaviour of the underlying system calls)
+      long_wpipe.close
+      assert_equal [long_pid], [t2, t3].map(&:value).compact
+    end
+  ensure
+    [t1, t2, t3].each { _1&.kill rescue nil }
+    [t1, t2, t3].each { _1&.join rescue nil }
+    [long_rpipe, long_wpipe, short_rpipe, short_wpipe].each { _1&.close rescue nil }
+  end if defined?(fork)
 end