(PUP-6675) Use pipes instead of temporary files for Puppet exec

acceptance/tests/resource/exec/should_accept_large_output.rb

@@ -0,0 +1,26 @@
+test_name "tests that puppet correctly captures large and empty output."
+
+agents.each do |agent|
+  testfile = agent.tmpfile('should_accept_large_output')
+
+  # Generate >64KB file to exceed pipe buffer.
+  lorem_ipsum = <<EOF
+Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna
+aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
+Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
+occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
+EOF
+  create_remote_file(agent, testfile, lorem_ipsum*1024)
+
+  apply_manifest_on(agent, "exec {'cat #{testfile}': path => ['/bin', '/usr/bin', 'C:/cygwin32/bin', 'C:/cygwin64/bin'], logoutput => true}") do
+    fail_test "didn't seem to run the command" unless
+      stdout.include? 'executed successfully'
+    fail_test "didn't print output correctly" unless
+      stdout.lines.select {|line| line =~ /\/returns:/}.count == 4097
+  end
+
+  apply_manifest_on(agent, "exec {'echo': path => ['/bin', '/usr/bin', 'C:/cygwin32/bin', 'C:/cygwin64/bin'], logoutput => true}") do
+    fail_test "didn't seem to run the command" unless
+      stdout.include? 'executed successfully'
+  end
+end

lib/puppet/util/execution.rb

@@ -187,18 +187,31 @@ def self.execute(command, options = NoOptionsSpecified)
     null_file = Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null'
 
     begin
+      # We close stdin/stdout/stderr immediately after execution as there no longer needed.
+      # In most cases they could be closed later, but when stdout is the writer pipe we
+      # must close it or we'll never reach eof on the reader.
+      reader, writer = IO.pipe unless options[:squelch]
       stdin = Puppet::FileSystem.open(options[:stdinfile] || null_file, nil, 'r')
-      stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : Puppet::FileSystem::Uniquefile.new('puppet')
+      stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : writer
       stderr = options[:combine] ? stdout : Puppet::FileSystem.open(null_file, nil, 'w')
 
       exec_args = [command, options, stdin, stdout, stderr]
+      output = ''
 
       if execution_stub = Puppet::Util::ExecutionStub.current_value
-        return execution_stub.call(*exec_args)
+        child_pid = execution_stub.call(*exec_args)
+        [stdin, stdout, stderr].each {|io| io.close rescue nil}
+        return child_pid
       elsif Puppet.features.posix?
         child_pid = nil
         begin
           child_pid = execute_posix(*exec_args)
+          [stdin, stdout, stderr].each {|io| io.close rescue nil}
+          unless options[:squelch]
+            while !reader.eof?
+              output << reader.read
+            end
+          end
           exit_status = Process.waitpid2(child_pid).last.exitstatus
           child_pid = nil
         rescue Timeout::Error => e
@@ -216,28 +229,28 @@ def self.execute(command, options = NoOptionsSpecified)
       elsif Puppet.features.microsoft_windows?
         process_info = execute_windows(*exec_args)
         begin
+          [stdin, stdout, stderr].each {|io| io.close rescue nil}
+          unless options[:squelch]
+            while !reader.eof?
+              output << reader.read
+            end
+          end
           exit_status = Puppet::Util::Windows::Process.wait_process(process_info.process_handle)
         ensure
           FFI::WIN32.CloseHandle(process_info.process_handle)
           FFI::WIN32.CloseHandle(process_info.thread_handle)
         end
       end
 
-      [stdin, stdout, stderr].each {|io| io.close rescue nil}
-
-      # read output in if required
-      unless options[:squelch]
-        output = wait_for_output(stdout)
-        Puppet.warning _("Could not get output") unless output
-      end
-
       if options[:failonfail] and exit_status != 0
         raise Puppet::ExecutionFailure, _("Execution of '%{str}' returned %{exit_status}: %{output}") % { str: command_str, exit_status: exit_status, output: output.strip }
       end
     ensure
-      if !options[:squelch] && stdout
-        # if we opened a temp file for stdout, we need to clean it up.
-        stdout.close!
+      # Make sure all handles are closed in case an exception was thrown attempting to execute.
+      [stdin, stdout, stderr].each {|io| io.close rescue nil}
+      if !options[:squelch] && reader
+        # if we opened a pipe, we need to clean it up.
+        reader.close
       end
     end
 
@@ -325,35 +338,4 @@ def self.execute_windows(command, options, stdin, stdout, stderr)
     end
   end
   private_class_method :execute_windows
-
-
-  # This is private method.
-  # @comment see call to private_class_method after method definition
-  # @api private
-  #
-  def self.wait_for_output(stdout)
-    # Make sure the file's actually been written.  This is basically a race
-    # condition, and is probably a horrible way to handle it, but, well, oh
-    # well.
-    # (If this method were treated as private / inaccessible from outside of this file, we shouldn't have to worry
-    #  about a race condition because all of the places that we call this from are preceded by a call to "waitpid2",
-    #  meaning that the processes responsible for writing the file have completed before we get here.)
-    2.times do |try|
-      if Puppet::FileSystem.exist?(stdout.path)
-        stdout.open
-        begin
-          return stdout.read
-        ensure
-          stdout.close
-          stdout.unlink
-        end
-      else
-        time_to_sleep = try / 2.0
-        Puppet.warning _("Waiting for output; will sleep %{time_to_sleep} seconds") % { time_to_sleep: time_to_sleep }
-        sleep(time_to_sleep)
-      end
-    end
-    nil
-  end
-  private_class_method :wait_for_output
 end

spec/unit/util/execution_spec.rb

@@ -29,6 +29,7 @@ def stub_process_wait(exitstatus)
       end
     end
 
+
     describe "#execute_posix (stubs)", :unless => Puppet.features.microsoft_windows? do
       before :each do
         # Most of the things this method does are bad to do during specs. :/
@@ -216,78 +217,49 @@ def stub_process_wait(exitstatus)
         end
 
         describe "when squelch is not set" do
-          it "should set stdout to a temporary output file" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
+          it "should set stdout to a pipe" do
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,_|
-              stdout.path == outfile.path
+              stdout.class == IO
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false)
           end
 
           it "should set stderr to the same file as stdout if combine is true" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == outfile.path
+              stdout == stderr
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false, :combine => true)
           end
 
           it "should set stderr to the null device if combine is false" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == null_file
+              stdout.class == IO and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false, :combine => false)
           end
 
-          it "should combine stdout and stderr if combine is true" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
-            Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == outfile.path
-            end.returns(rval)
-
-            Puppet::Util::Execution.execute('test command', :combine => true)
-          end
-
           it "should default combine to true when no options are specified" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == outfile.path
+              stdout == stderr
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command')
           end
 
           it "should default combine to false when options are specified, but combine is not" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == null_file
+              stdout.class == IO and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :failonfail => false)
           end
 
           it "should default combine to false when an empty hash of options is specified" do
-            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
-            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
-
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.path == outfile.path and stderr.path == null_file
+              stdout.class == IO and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', {})
@@ -536,9 +508,10 @@ def stub_process_wait(exitstatus)
       end
 
       it "should close the stdin/stdout/stderr files used by the child" do
-        stdin = mock 'file', :close
-        stdout = mock 'file', :close
-        stderr = mock 'file', :close
+        stdin = mock 'file'
+        stdout = mock 'file'
+        stderr = mock 'file'
+        [stdin, stdout, stderr].each {|io| io.expects(:close).at_least_once}
 
         File.expects(:open).
             times(3).
@@ -550,37 +523,43 @@ def stub_process_wait(exitstatus)
       end
 
       it "should read and return the output if squelch is false" do
-        stdout = Puppet::FileSystem::Uniquefile.new('test')
-        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
-        stdout.write("My expected command output")
+        r, w = IO.pipe
+        IO.expects(:pipe).returns([r, w])
+        w.write("My expected command output")
 
         expect(Puppet::Util::Execution.execute('test command')).to eq("My expected command output")
       end
 
       it "should not read the output if squelch is true" do
-        stdout = Puppet::FileSystem::Uniquefile.new('test')
-        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
-        stdout.write("My expected command output")
+        IO.expects(:pipe).never
 
         expect(Puppet::Util::Execution.execute('test command', :squelch => true)).to eq('')
       end
 
-      it "should delete the file used for output if squelch is false" do
-        stdout = Puppet::FileSystem::Uniquefile.new('test')
-        path = stdout.path
-        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
-
-        Puppet::Util::Execution.execute('test command')
+      it "should close the pipe used for output if squelch is false" do
+        r, w = IO.pipe
+        IO.expects(:pipe).returns([r, w])
 
-        expect(Puppet::FileSystem.exist?(path)).to be_falsey
+        expect(Puppet::Util::Execution.execute('test command')).to eq("")
+        expect(r.closed?)
+        expect(w.closed?)
       end
 
-      it "should not raise an error if the file is open" do
-        stdout = Puppet::FileSystem::Uniquefile.new('test')
-        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
-        file = File.new(stdout.path, 'r')
+      it "should close the pipe used for output if squelch is false and an error is raised" do
+        r, w = IO.pipe
+        IO.expects(:pipe).returns([r, w])
 
-        Puppet::Util::Execution.execute('test command')
+        if Puppet.features.microsoft_windows?
+          Puppet::Util::Execution.expects(:execute_windows).raises(Exception, 'execution failed')
+        else
+          Puppet::Util::Execution.expects(:execute_posix).raises(Exception, 'execution failed')
+        end
+
+        expect {
+          subject.execute('fail command')
+        }.to raise_error(Exception, 'execution failed')
+        expect(r.closed?)
+        expect(w.closed?)
       end
 
       it "should raise an error if failonfail is true and the child failed" do