Merge pull request #12 from opscode/ss/OC-7160

Clean inherited file descriptors from the parent while forking subprocess.

2 files changed, 35 insertions, 3 deletions

diff --git a/lib/mixlib/shellout/unix.rb b/lib/mixlib/shellout/unix.rb
index 776169d..d7082e1 100644
--- a/lib/mixlib/shellout/unix.rb
+++ b/lib/mixlib/shellout/unix.rb
@@ -178,6 +178,26 @@ module Mixlib
         STDIN.sync = true if input
       end
 
+      # When a new process is started with chef, it shares the file
+      # descriptors of the parent. We clean the file descriptors
+      # coming from the parent to prevent unintended locking if parent
+      # is killed.
+      # NOTE: After some discussions we've decided to iterate on file
+      # descriptors upto 256. We believe this  is a reasonable upper
+      # limit in a chef environment. If we have issues in the future this
+      # number could be made to be configurable or updated based on
+      # the ulimit based on platform.
+      def clean_parent_file_descriptors
+        # Don't clean $stdin, $stdout, $stderr, process_status_pipe.
+        # Also 3 & 4 is reserved by RubyVM
+        5.upto(256) do |n|
+          fd = File.for_fd(n) rescue nil
+          if fd && process_status_pipe.last.to_i != n
+            fd.close
+          end
+        end
+      end
+
       def configure_parent_process_file_descriptors
         # Close the sides of the pipes we don't care about
         stdin_pipe.first.close
@@ -231,6 +251,8 @@ module Mixlib
         fork do
           configure_subprocess_file_descriptors
 
+          clean_parent_file_descriptors
+
           set_group
           set_user
           set_environment
diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb
index 14af6e2..e4fe504 100644
--- a/spec/mixlib/shellout_spec.rb
+++ b/spec/mixlib/shellout_spec.rb
@@ -203,7 +203,7 @@ describe Mixlib::ShellOut do
 
     context "with options hash" do
       let(:cmd) { 'brew install couchdb' }
-      let(:options) { { :cwd => cwd, :user => user, :domain => domain, :password => password, :group => group, 
+      let(:options) { { :cwd => cwd, :user => user, :domain => domain, :password => password, :group => group,
         :umask => umask, :timeout => timeout, :environment => environment, :returns => valid_exit_codes,
         :live_stream => stream, :input => input } }
 
@@ -422,12 +422,12 @@ describe Mixlib::ShellOut do
         let(:user) { 'administrator' }
         let(:domain) { ENV['COMPUTERNAME'].downcase }
         let(:options) { { :domain => domain, :user => user, :password => 'vagrant' } }
-  
+
         it "should run as Administrator" do
           running_user.should eql("#{domain}\\#{user}")
         end
       end
-    end      
+    end
 
     context "with a live stream" do
       let(:stream) { StringIO.new }
@@ -762,6 +762,16 @@ describe Mixlib::ShellOut do
         end
       end
 
+      context 'with open files for parent process' do
+        let(:ruby_code) { "count = 0; 0.upto(256) do |n| fd = File.for_fd(n) rescue nil; count += 1 if fd end; puts count" }
+
+        it "should not see file descriptors of the parent" do
+          test_file = Tempfile.new('fd_test')
+          stdout.chomp.should eql("3")
+          test_file.close
+        end
+      end
+
       context 'with subprocess that takes longer than timeout' do
         let(:cmd) { ruby_eval.call('sleep 2') }
         let(:options) { { :timeout => 0.1 } }