Close Open3 pipes when spawn exits early

Prevent file descriptor leaks when process creation exits through an
exception or throw.

Author: nobu

lib/open3.rb

@@ -520,30 +520,30 @@ def popen2e(*cmd, &block)
     opts[[:out, :err]] = out_w
 
     popen_run(cmd, opts, [in_r, out_w], [in_w, out_r], &block)
-  ensure
-    if block
-      in_r.close
-      in_w.close
-      out_r.close
-      out_w.close
-    end
   end
   module_function :popen2e
 
   def popen_run(cmd, opts, child_io, parent_io) # :nodoc:
     pid = spawn(*cmd, opts)
     wait_thr = Process.detach(pid)
     child_io.each(&:close)
+    child_io = nil
     result = [*parent_io, wait_thr]
     if defined? yield
-      begin
-        return yield(*result)
-      ensure
+      yield(*result)
+    else
+      result
+    end
+  ensure
+    if result
+      if defined? yield
         parent_io.each(&:close)
         wait_thr.join
       end
+    else
+      child_io&.each(&:close)
+      parent_io.each(&:close)
     end
-    result
   end
   module_function :popen_run
   class << self
@@ -1354,7 +1354,7 @@ def pipeline_run(cmds, pipeline_opts, child_io, parent_io) # :nodoc:
     opts_base.delete :out
 
     wait_thrs = []
-    r = nil
+    r = r2 = w2 = nil
     cmds.each_with_index {|cmd, i|
       cmd_opts = opts_base.dup
       if String === cmd
@@ -1387,17 +1387,25 @@ def pipeline_run(cmds, pipeline_opts, child_io, parent_io) # :nodoc:
       w2&.close
       r = r2
     }
-    result = parent_io + [wait_thrs]
     child_io.each(&:close)
+    child_io = nil
+    result = parent_io + [wait_thrs]
     if defined? yield
-      begin
-        return yield(*result)
-      ensure
+      yield(*result)
+    else
+      result
+    end
+  ensure
+    if result
+      if defined? yield
         parent_io.each(&:close)
         wait_thrs.each(&:join)
       end
+    else
+      [r, r2, w2, *child_io, *parent_io].each do |io|
+        io&.close
+      end
     end
-    result
   end
   module_function :pipeline_run
   class << self

test/test_open3.rb

@@ -159,6 +159,67 @@ def test_popen2e_noblock
     t.join
   end
 
+  def test_popen_spawn_failure_closes_pipes
+    [:popen3, :popen2, :popen2e].each do |method|
+      assert_no_fd_leak(method) do
+        assert_raise(Errno::ENOENT) do
+          Open3.public_send(method, "/open3-command-does-not-exist")
+        end
+      end
+    end
+  end
+
+  def test_popen_spawn_throw_closes_pipes
+    tag = Object.new
+    assert_no_fd_leak(:popen3) do
+      stub_open3_spawn(->(*) {throw tag}) do
+        assert_throw(tag) do
+          Open3.popen3("unused")
+        end
+      end
+    end
+  end
+
+  def test_pipeline_spawn_failure_closes_pipes
+    first = [RUBY, '-e', '']
+    missing = ["/open3-command-does-not-exist"]
+
+    [:pipeline_rw, :pipeline_r, :pipeline_w,
+     :pipeline_start, :pipeline].each do |method|
+      threads = Thread.list
+      assert_no_fd_leak(method) do
+        assert_raise(Errno::ENOENT) do
+          Open3.public_send(method, first, missing)
+        end
+      end
+      (Thread.list - threads).each do |thread|
+        thread.join if Process::Waiter === thread
+      end
+    end
+  end
+
+  def test_pipeline_spawn_throw_closes_pipes
+    tag = Object.new
+    spawn = Open3.method(:spawn)
+    calls = 0
+    threads = Thread.list
+
+    assert_no_fd_leak(:pipeline_rw) do
+      stub_open3_spawn(->(*args) {
+        calls += 1
+        throw tag if calls == 2
+        spawn.call(*args)
+      }) do
+        assert_throw(tag) do
+          Open3.pipeline_rw([RUBY, '-e', ''], ["unused"])
+        end
+      end
+    end
+    (Thread.list - threads).each do |thread|
+      thread.join if Process::Waiter === thread
+    end
+  end
+
   def test_capture3
     o, e, s = Open3.capture3(RUBY, '-e', 'i=STDIN.read; print i+"o"; STDOUT.flush; STDERR.print i+"e"', :stdin_data=>"i")
     assert_equal("io", o)
@@ -332,4 +393,33 @@ def test_integer_and_symbol_key
     assert_equal("test_integer_and_symbol_key\n", out)
     assert_predicate(status, :success?)
   end
+
+  private
+
+  def stub_open3_spawn(spawn)
+    Open3.define_singleton_method(:spawn, spawn)
+    yield
+  ensure
+    Open3.singleton_class.send(:remove_method, :spawn)
+  end
+
+  def assert_no_fd_leak(method)
+    fd_dir = ["/proc/self/fd", "/dev/fd"].find {|dir| File.directory?(dir) }
+    omit "cannot inspect open file descriptors" unless fd_dir
+
+    gc_was_disabled = GC.disable
+    before = open_fds(fd_dir)
+    yield
+    assert_equal(before, open_fds(fd_dir),
+                 "#{method} leaked file descriptors")
+  ensure
+    GC.enable unless gc_was_disabled
+  end
+
+  def open_fds(fd_dir)
+    Dir.open(fd_dir) do |dir|
+      fds = dir.children(&:to_i).sort
+      fds -= [dir.fileno] if dir.respond_to? :fileno
+    end
+  end
 end