FD_CLOEXEC all non-listen descriptors before exec

We'll allow before_exec to override that setting, however.

There are cases where someone setting
Logger.new("/path/to/file") will create new file descriptors in
the master process.  This will prevent FD leakage
and a test case (for Linux only) proves it.

2 files changed, 77 insertions, 5 deletions

diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 00012f7..c1d68c1 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -326,15 +326,24 @@ module Unicorn
       end
 
       @reexec_pid = fork do
-        @rd_sig.close if @rd_sig
-        @wr_sig.close if @wr_sig
-        @workers.values.each { |other| other.tempfile.close rescue nil }
-
         ENV.replace(@start_ctx[:environ])
-        ENV['UNICORN_FD'] = @listeners.map { |sock| sock.fileno }.join(',')
+        listener_fds = @listeners.map { |sock| sock.fileno }
+        ENV['UNICORN_FD'] = listener_fds.join(',')
         File.umask(@start_ctx[:umask])
         Dir.chdir(@start_ctx[:cwd])
         cmd = [ @start_ctx[:zero] ] + @start_ctx[:argv]
+
+        # avoid leaking FDs we don't know about, but let before_exec
+        # unset FD_CLOEXEC, if anything else in the app eventually
+        # relies on FD inheritence.
+        purgatory = [] # prevent GC of IO objects
+        (3..1024).each do |io|
+          next if listener_fds.include?(io)
+          io = IO.for_fd(io) rescue nil
+          io or next
+          purgatory << io
+          io.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)
+        end
         logger.info "executing #{cmd.inspect} (in #{Dir.pwd})"
         @before_exec.call(self) if @before_exec
         exec(*cmd)
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index 78f452b..6c3d282 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -499,4 +499,67 @@ end
     reexec_usr2_quit_test(new_pid, pid_file)
   end
 
+  def test_reexec_fd_leak
+    unless RUBY_PLATFORM =~ /linux/ # Solaris may work, too, but I forget...
+      warn "FD leak test only works on Linux at the moment"
+      return
+    end
+    pid_file = "#{@tmpdir}/test.pid"
+    log = Tempfile.new('unicorn_test_log')
+    log.sync = true
+    ucfg = Tempfile.new('unicorn_test_config')
+    ucfg.syswrite("pid \"#{pid_file}\"\n")
+    ucfg.syswrite("logger Logger.new('#{log.path}')\n")
+    ucfg.syswrite("stderr_path '#{log.path}'\n")
+    ucfg.syswrite("stdout_path '#{log.path}'\n")
+    ucfg.close
+
+    File.open("config.ru", "wb") { |fp| fp.syswrite(HI) }
+    pid = xfork do
+      redirect_test_io do
+        exec($unicorn_bin, "-D", "-l#{@addr}:#{@port}", "-c#{ucfg.path}")
+      end
+    end
+
+    wait_master_ready(log.path)
+    wait_for_file(pid_file)
+    orig_pid = pid = File.read(pid_file).to_i
+    orig_fds = `ls -l /proc/#{pid}/fd`.split(/\n/)
+    assert $?.success?
+    expect_size = orig_fds.size
+
+    assert_nothing_raised do
+      Process.kill(:USR2, pid)
+      wait_for_file("#{pid_file}.oldbin")
+      Process.kill(:QUIT, pid)
+    end
+    wait_for_death(pid)
+
+    wait_for_file(pid_file)
+    pid = File.read(pid_file).to_i
+    assert_not_equal orig_pid, pid
+    curr_fds = `ls -l /proc/#{pid}/fd`.split(/\n/)
+    assert $?.success?
+
+    # we could've inherited descriptors the first time around
+    assert expect_size >= curr_fds.size
+    expect_size = curr_fds.size
+
+    assert_nothing_raised do
+      Process.kill(:USR2, pid)
+      wait_for_file("#{pid_file}.oldbin")
+      Process.kill(:QUIT, pid)
+    end
+    wait_for_death(pid)
+
+    wait_for_file(pid_file)
+    pid = File.read(pid_file).to_i
+    curr_fds = `ls -l /proc/#{pid}/fd`.split(/\n/)
+    assert $?.success?
+    assert_equal expect_size, curr_fds.size
+
+    Process.kill(:QUIT, pid)
+    wait_for_death(pid)
+  end
+
 end if do_test