diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index fe763710e85..80ce96339ea 100644 --- a/lib/puppet/util.rb +++ b/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,6 +242,41 @@ 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 @@ -248,13 +284,10 @@ def execfail(command, exception) # 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,99 +296,35 @@ 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 @@ -363,6 +332,27 @@ def execute(command, arguments = {:failonfail => true, :combine => true}) 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 } diff --git a/spec/integration/util_spec.rb b/spec/integration/util_spec.rb new file mode 100644 index 00000000000..a50f783260a --- /dev/null +++ b/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 diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb index e0658321527..4481f38637f 100644 --- a/spec/unit/util_spec.rb +++ b/spec/unit/util_spec.rb @@ -46,4 +46,290 @@ end end end + + describe "execution methods" do + let(:pid) { 5501 } + let(:null_file) { Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null' } + + describe "#execute_posix" do + before :each do + # Most of the things this method does are bad to do during specs. :/ + Kernel.stubs(:fork).returns(pid).yields + Process.stubs(:setsid) + Kernel.stubs(:exec) + Puppet::Util::SUIDManager.stubs(:change_user) + Puppet::Util::SUIDManager.stubs(:change_group) + + $stdin.stubs(:reopen) + $stdout.stubs(:reopen) + $stderr.stubs(:reopen) + + @stdin = File.open(null_file, 'r') + @stdout = Tempfile.new('stdout') + @stderr = File.open(null_file, 'w') + end + + it "should fork a child process to execute the command" do + Kernel.expects(:fork).returns(pid).yields + Kernel.expects(:exec).with('test command') + + Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr) + end + + it "should start a new session group" do + Process.expects(:setsid) + + Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr) + end + + it "should close all open file descriptors except stdin/stdout/stderr" do + # This is ugly, but I can't really think of a better way to do it without + # letting it actually close fds, which seems risky + (0..2).each {|n| IO.expects(:new).with(n).never} + (3..256).each {|n| IO.expects(:new).with(n).returns mock('io', :close) } + + Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr) + end + + it "should permanently change to the correct user and group if specified" do + Puppet::Util::SUIDManager.expects(:change_group).with(55, true) + Puppet::Util::SUIDManager.expects(:change_user).with(50, true) + + Puppet::Util.execute_posix('test command', {:uid => 50, :gid => 55}, @stdin, @stdout, @stderr) + end + + it "should exit failure if there is a problem execing the command" do + Kernel.expects(:exec).with('test command').raises("failed to execute!") + Puppet::Util.stubs(:puts) + Puppet::Util.expects(:exit!).with(1) + + Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr) + end + + it "should properly execute commands specified as arrays" do + Kernel.expects(:exec).with('test command', 'with', 'arguments') + + Puppet::Util.execute_posix(['test command', 'with', 'arguments'], {:uid => 50, :gid => 55}, @stdin, @stdout, @stderr) + end + + it "should return the pid of the child process" do + Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr).should == pid + end + end + + describe "#execute_windows" do + let(:proc_info_stub) { stub 'processinfo', :process_id => pid } + + before :each do + Process.stubs(:create).returns(proc_info_stub) + Process.stubs(:waitpid2).with(pid).returns([pid, 0]) + + @stdin = File.open(null_file, 'r') + @stdout = Tempfile.new('stdout') + @stderr = File.open(null_file, 'w') + end + + it "should create a new process for the command" do + Process.expects(:create).with( + :command_line => "test command", + :startup_info => {:stdin => @stdin, :stdout => @stdout, :stderr => @stderr} + ).returns(proc_info_stub) + + Puppet::Util.execute_windows('test command', {}, @stdin, @stdout, @stderr) + end + + it "should return the pid of the child process" do + Puppet::Util.execute_windows('test command', {}, @stdin, @stdout, @stderr).should == pid + end + + it "should quote arguments containing spaces if command is specified as an array" do + Process.expects(:create).with do |args| + args[:command_line] == '"test command" with some "arguments \"with spaces"' + end.returns(proc_info_stub) + + Puppet::Util.execute_windows(['test command', 'with', 'some', 'arguments "with spaces'], {}, @stdin, @stdout, @stderr) + end + end + + describe "#execute" do + before :each do + Process.stubs(:waitpid2).with(pid).returns([pid, 0]) + end + + describe "when an execution stub is specified" do + before :each do + Puppet::Util::ExecutionStub.set do |command,args,stdin,stdout,stderr| + "execution stub output" + end + end + + it "should call the block on the stub" do + Puppet::Util.execute("/usr/bin/run_my_execute_stub").should == "execution stub output" + end + + it "should not actually execute anything" do + Puppet::Util.expects(:execute_posix).never + Puppet::Util.expects(:execute_windows).never + + Puppet::Util.execute("/usr/bin/run_my_execute_stub") + end + end + + describe "when setting up input and output files" do + include PuppetSpec::Files + let(:executor) { Puppet.features.microsoft_windows? ? 'execute_windows' : 'execute_posix' } + + before :each do + Puppet::Util.stubs(:wait_for_output) + end + + it "should set stdin to the stdinfile if specified" do + input = tmpfile('stdin') + FileUtils.touch(input) + + Puppet::Util.expects(executor).with do |_,_,stdin,_,_| + stdin.path == input + end.returns(pid) + + Puppet::Util.execute('test command', :stdinfile => input) + end + + it "should set stdin to the null file if not specified" do + Puppet::Util.expects(executor).with do |_,_,stdin,_,_| + stdin.path == null_file + end.returns(pid) + + Puppet::Util.execute('test command') + end + + describe "when squelch is set" do + it "should set stdout and stderr to the null file" do + Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr| + stdout.path == null_file and stderr.path == null_file + end.returns(pid) + + Puppet::Util.execute('test command', :squelch => true) + end + end + + describe "when squelch is not set" do + it "should set stdout to a temporary output file" do + outfile = Tempfile.new('stdout') + Tempfile.stubs(:new).returns(outfile) + + Puppet::Util.expects(executor).with do |_,_,_,stdout,_| + stdout.path == outfile.path + end.returns(pid) + + Puppet::Util.execute('test command', :squelch => false) + end + + it "should set stderr to the same file as stdout if combine is true" do + outfile = Tempfile.new('stdout') + Tempfile.stubs(:new).returns(outfile) + + Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr| + stdout.path == outfile.path and stderr.path == outfile.path + end.returns(pid) + + Puppet::Util.execute('test command', :squelch => false, :combine => true) + end + + it "should set stderr to the null device if combine is false" do + outfile = Tempfile.new('stdout') + Tempfile.stubs(:new).returns(outfile) + + Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr| + stdout.path == outfile.path and stderr.path == null_file + end.returns(pid) + + Puppet::Util.execute('test command', :squelch => false, :combine => false) + end + end + end + end + + describe "after execution" do + let(:executor) { Puppet.features.microsoft_windows? ? 'execute_windows' : 'execute_posix' } + before :each do + Process.stubs(:waitpid2).with(pid).returns([pid, 0]) + + Puppet::Util.stubs(executor).returns(pid) + end + + it "should wait for the child process to exit" do + Puppet::Util.stubs(:wait_for_output) + + Process.expects(:waitpid2).with(pid).returns([pid, 0]) + + Puppet::Util.execute('test command') + 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 + + File.expects(:open). + times(3). + returns(stdin). + then.returns(stdout). + then.returns(stderr) + + Puppet::Util.execute('test command', :squelch => true) + end + + it "should read and return the output if squelch is false" do + stdout = Tempfile.new('test') + Tempfile.stubs(:new).returns(stdout) + stdout.write("My expected command output") + + Puppet::Util.execute('test command').should == "My expected command output" + end + + it "should not read the output if squelch is true" do + stdout = Tempfile.new('test') + Tempfile.stubs(:new).returns(stdout) + stdout.write("My expected command output") + + Puppet::Util.execute('test command', :squelch => true).should == nil + end + + it "should delete the file used for output if squelch is false" do + stdout = Tempfile.new('test') + Tempfile.stubs(:new).returns(stdout) + + Puppet::Util.execute('test command') + + # Tempfile#path returns nil if the file has been unlinked + stdout.path.should == nil + end + + it "should raise an error if failonfail is true and the child failed" do + child_status = stub('child_status', :exitstatus => 1) + + Process.expects(:waitpid2).with(pid).returns([pid, child_status]) + + expect { + Puppet::Util.execute('fail command', :failonfail => true) + }.to raise_error(Puppet::ExecutionFailure, /Execution of 'fail command' returned 1/) + end + + it "should not raise an error if failonfail is false and the child failed" do + Process.expects(:waitpid2).with(pid).returns([pid, 1]) + + expect { + Puppet::Util.execute('fail command', :failonfail => false) + }.not_to raise_error + end + + it "should not raise an error if failonfail is true and the child succeeded" do + Process.expects(:waitpid2).with(pid).returns([pid, 0]) + + expect { + Puppet::Util.execute('fail command', :failonfail => true) + }.not_to raise_error + end + end + end end diff --git a/test/util/utiltest.rb b/test/util/utiltest.rb index 1c934d61219..9bc243c1c4f 100755 --- a/test/util/utiltest.rb +++ b/test/util/utiltest.rb @@ -219,18 +219,5 @@ def test_lang_environ_in_execute end end - - # Check whether execute accepts strings in addition to arrays. - def test_string_exec - cmd = "/bin/echo howdy" - output = nil - assert_raise(ArgumentError) { - output = Puppet::Util.execute(cmd) - } - #assert_equal("howdy\n", output) - #assert_raise(RuntimeError) { - # Puppet::Util.execute(cmd, 0, 0) - #} - end end