(#8410) Cleanup and fix Windows support in Puppet::Util.execute

The primary change in this commit is fixing the support for Windows in
Puppet::Util.execute. However, properly testing this commit required
significant refactoring (which was long overdue for this old code) for
testability.

The functionality for executing on posix and windows was extracted into
platform-specific methods, execute_posix and execute_windows. These
methods are self-contained and will both return the PID of the child
process, which the caller then waits on before reading and returning the
output of the command.

lib/puppet/util.rb

@@ -3,6 +3,7 @@
 require 'English'
 require 'puppet/util/monkey_patches'
 require 'sync'
+require 'tempfile'
 require 'puppet/external/lock'
 require 'monitor'
 require 'puppet/util/execution_stub'
@@ -241,20 +242,52 @@ def execfail(command, exception)
       raise exception, output
   end
 
+  def execute_posix(command, arguments, stdin, stdout, stderr)
+    child_pid = Kernel.fork do
+      command = Array(command)
+      Process.setsid
+      begin
+        $stdin.reopen(stdin)
+        $stdout.reopen(stdout)
+        $stderr.reopen(stderr)
+
+        3.upto(256){|fd| IO::new(fd).close rescue nil}
+
+        Puppet::Util::SUIDManager.change_group(arguments[:gid], true) if arguments[:gid]
+        Puppet::Util::SUIDManager.change_user(arguments[:uid], true)  if arguments[:uid]
+
+        ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C'
+        Kernel.exec(*command)
+      rescue => detail
+        puts detail.to_s
+        exit!(1)
+      end
+    end
+    child_pid
+  end
+  module_function :execute_posix
+
+  def execute_windows(command, arguments, stdin, stdout, stderr)
+    command = command.map do |part|
+      part.include?(' ') ? %Q["#{part.gsub(/"/, '\"')}"] : part
+    end.join(" ") if command.is_a?(Array)
+
+    process_info = Process.create( :command_line => command, :startup_info => {:stdin => stdin, :stdout => stdout, :stderr => stderr} )
+    process_info.process_id
+  end
+  module_function :execute_windows
+
   # Execute the desired command, and return the status and output.
   # def execute(command, failonfail = true, uid = nil, gid = nil)
   # :combine sets whether or not to combine stdout/stderr in the output
   # :stdinfile sets a file that can be used for stdin. Passing a string
   # for stdin is not currently supported.
   def execute(command, arguments = {:failonfail => true, :combine => true})
     if command.is_a?(Array)
-      command = command.flatten.collect { |i| i.to_s }
+      command = command.flatten.map(&:to_s)
       str = command.join(" ")
-    else
-      # We require an array here so we know where we're incorrectly
-      # using a string instead of an array.  Once everything is
-      # switched to an array, we might relax this requirement.
-      raise ArgumentError, "Must pass an array to execute()"
+    elsif command.is_a?(String)
+      str = command
     end
 
     if respond_to? :debug
@@ -263,106 +296,63 @@ def execute(command, arguments = {:failonfail => true, :combine => true})
       Puppet.debug "Executing '#{str}'"
     end
 
-    if execution_stub = Puppet::Util::ExecutionStub.current_value
-      return execution_stub.call(command, arguments)
-    end
+    null_file = Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null'
 
-    @@os ||= Facter.value(:operatingsystem)
-    output = nil
-    child_pid, child_status = nil
-    # There are problems with read blocking with badly behaved children
-    # read.partialread doesn't seem to capture either stdout or stderr
-    # We hack around this using a temporary file
-
-    # The idea here is to avoid IO#read whenever possible.
-    output_file="/dev/null"
-    error_file="/dev/null"
-    if ! arguments[:squelch]
-      require "tempfile"
-      output_file = Tempfile.new("puppet")
-      error_file=output_file if arguments[:combine]
-    end
+    stdin = File.open(arguments[:stdinfile] || null_file, 'r')
+    stdout = arguments[:squelch] ? File.open(null_file, 'w') : Tempfile.new('puppet')
+    stderr = arguments[:combine] ? stdout : File.open(null_file, 'w')
 
-    if Puppet.features.posix?
-      oldverb = $VERBOSE
-      $VERBOSE = nil
-      child_pid = Kernel.fork
-      $VERBOSE = oldverb
-      if child_pid
-        # Parent process executes this
-        child_status = (Process.waitpid2(child_pid)[1]).to_i >> 8
-      else
-        # Child process executes this
-        Process.setsid
-        begin
-          if arguments[:stdinfile]
-            $stdin.reopen(arguments[:stdinfile])
-          else
-            $stdin.reopen("/dev/null")
-          end
-          $stdout.reopen(output_file)
-          $stderr.reopen(error_file)
-
-          3.upto(256){|fd| IO::new(fd).close rescue nil}
-          Puppet::Util::SUIDManager.change_group(arguments[:gid], true) if arguments[:gid]
-          Puppet::Util::SUIDManager.change_user(arguments[:uid], true) if arguments[:uid]
-          ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C'
-          if command.is_a?(Array)
-            Kernel.exec(*command)
-          else
-            Kernel.exec(command)
-          end
-        rescue => detail
-          puts detail.to_s
-          exit!(1)
-        end
-      end
+
+    exec_args = [command, arguments, stdin, stdout, stderr]
+
+    if execution_stub = Puppet::Util::ExecutionStub.current_value
+      return execution_stub.call(*exec_args)
+    elsif Puppet.features.posix?
+      child_pid = execute_posix(*exec_args)
     elsif Puppet.features.microsoft_windows?
-      command = command.collect {|part| '"' + part.gsub(/"/, '\\"') + '"'}.join(" ") if command.is_a?(Array)
-      Puppet.debug "Creating process '#{command}'"
-      processinfo = Process.create( :command_line => command )
-      child_status = (Process.waitpid2(child_pid)[1]).to_i >> 8
+      child_pid = execute_windows(*exec_args)
     end
 
-    # read output in if required
-    if ! arguments[:squelch]
-
-      # Make sure the file's actually there.  This is
-      # basically a race condition, and is probably a horrible
-      # way to handle it, but, well, oh well.
-      unless FileTest.exists?(output_file.path)
-        Puppet.warning "sleeping"
-        sleep 0.5
-        unless FileTest.exists?(output_file.path)
-          Puppet.warning "sleeping 2"
-          sleep 1
-          unless FileTest.exists?(output_file.path)
-            Puppet.warning "Could not get output"
-            output = ""
-          end
-        end
-      end
-      unless output
-        # We have to explicitly open here, so that it reopens
-        # after the child writes.
-        output = output_file.open.read
+    child_status = Process.waitpid2(child_pid).last
 
-        # The 'true' causes the file to get unlinked right away.
-        output_file.close(true)
-      end
+    [stdin, stdout, stderr].each {|io| io.close rescue nil}
+
+    # read output in if required
+    unless arguments[:squelch]
+      output = wait_for_output(stdout)
+      Puppet.warning "Could not get output" unless output
     end
 
-    if arguments[:failonfail]
-      unless child_status == 0
-        raise ExecutionFailure, "Execution of '#{str}' returned #{child_status}: #{output}"
-      end
+    if arguments[:failonfail] and child_status != 0
+      raise ExecutionFailure, "Execution of '#{str}' returned #{child_status.exitstatus}: #{output}"
     end
 
     output
   end
 
   module_function :execute
 
+  def 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.
+    2.times do |try|
+      if File.exists?(stdout.path)
+        output = stdout.open.read
+
+        stdout.close(true)
+
+        return output
+      else
+        time_to_sleep = try / 2.0
+        Puppet.warning "Waiting for output; will sleep #{time_to_sleep} seconds"
+        sleep(time_to_sleep)
+      end
+    end
+    nil
+  end
+  module_function :wait_for_output
+
   # Create an exclusive lock.
   def threadlock(resource, type = Sync::EX)
     Puppet::Util.synchronize_on(resource,type) { yield }

spec/integration/util_spec.rb

@@ -0,0 +1,13 @@
+#!/usr/bin/env ruby
+
+require 'spec_helper'
+
+describe Puppet::Util do
+  describe "#execute" do
+    it "should properly allow stdout and stderr to share a file" do
+      command = "ruby -e '(1..10).each {|i| (i%2==0) ? $stdout.puts(i) : $stderr.puts(i)}'"
+
+      Puppet::Util.execute(command, :combine => true).split.should =~ [*'1'..'10']
+    end
+  end
+end