The recent release of 0.25.2 contains a new method of handling communication with "exec"ed processes to support SELinux and improve performance on other OSes which, unfortunately, has a number of shortcomings:
* It does not work on ruby < 1.8.3 * It reintroduces one/possibly two bugs * It has no tests This patch addresses these problems (and an analogous issue with SELinux mount handling discovered in the process). The structure of the patch: * Tests for the base cases and applicable bugs from the ticket history * Code to select between readpartial (on ruby >=1.8.3) and sysread (<1.8.3) * Logic to detect and deal with first and second order failures/misbehavior in the "exec"ed process * Logic to handle asynchronous signals (also ported to the SELinux mount code) * Notes / tests (pending, but written) for the remaining, unhandled third- order failure case This last bears some elaboration; if the following steps occur: * Puppet "exec"s child process (apt-get, for example) and attempts to capture the output * The child process launches a "grandchild" process (the buggy version of the osirid daemon under Debian Etch, for example) without closing it's STDOUT * The grandchild does not close its inherited STDOUT * The child exits * The grandchild does not trap SIGPIPE * The grandchild writes to STDOUT the grandchild will receive an (unhandled) SIGPIPE and exit. There is no presently known case where this would occur (the osirid daemon, as best as I can determine, did not actually write to the handle, just kept it around out of misplaced sentimentality). The solutions to this problem, if it exists, are messy and best left unimplemented unless they are needed. Signed-off-by: Markus Roberts <[email protected]> --- lib/puppet/util.rb | 48 ++++++++++--- lib/puppet/util/selinux.rb | 6 +- spec/unit/exec_test_helper | 45 +++++++++++ spec/unit/util.rb | 177 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 265 insertions(+), 11 deletions(-) create mode 100755 spec/unit/exec_test_helper create mode 100755 spec/unit/util.rb diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index e2df5c3..a37a917 100644 --- a/lib/puppet/util.rb +++ b/lib/puppet/util.rb @@ -266,24 +266,50 @@ module Util child_pid = Kernel.fork $VERBOSE = oldverb if child_pid + # Parent process executes this output_write.close - + reaped_pid = nil # Read output in if required if ! arguments[:squelch] output = '' begin - loop do - output << output_read.readpartial(4096) - end + method = output_read.respond_to?(:readpartial) ? :readpartial : :sysread + begin + # The timeout needs to be high enough to avoid thrashing but low + # enough that we don't waste too much time waiting for processes + # that exit without closing the pipe (see ticket #1563 comment #7) + # One second seems a reasonable compromise. + output << timeout(1) { output_read.send(method,1) } while true + rescue Timeout::Error + break if reaped_pid = Process.waitpid(child_pid,Process::WNOHANG) + rescue Errno::EINTR + # Just popping out to handle an unrelated signal, but we're not done + end while true rescue EOFError # End of file ensure - output_read.close + willing_to_abandon_pipes = false + # + # Setting this to true allows the 'exec'ed process to spawn a new process, + # pass it the stdout handle, exit, and have the new process write to the + # handle without getting a SIGPIPE so long as we're still running, at the + # cost of our leaking handles. It's set to false because even the notorious + # bug #512055 of osirid (see our ticket #1563 comment #7 for details) would + # not have needed this level of coddling. + # + # If it is needed, passing output_read to a separate light-weight reaper + # process that waits for the other end to close and then exits may be a + # preferable alternative. + # + if reaped_pid and willing_to_abandon_pipes + $abandoned_pipes ||= {} + $abandoned_pipes[reaped_pid] = output_read + else + output_read.close + end end end - - # Parent process executes this - Process.waitpid(child_pid) + reaped_pid ||= Process.waitpid(child_pid) child_status = $?.exitstatus else # Child process executes this @@ -321,9 +347,11 @@ module Util end ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C' Kernel.exec(*command) - rescue => detail + rescue Object => detail puts detail - exit(1) + $stdout.flush + ensure + exit!(1) end end diff --git a/lib/puppet/util/selinux.rb b/lib/puppet/util/selinux.rb index 3801ecd..1d671b7 100644 --- a/lib/puppet/util/selinux.rb +++ b/lib/puppet/util/selinux.rb @@ -156,7 +156,11 @@ module Puppet::Util::SELinux # If possible we use read_nonblock() in a loop rather than read() to work- # a linux kernel bug. See ticket #1963 for details. mountfh = File.open("/proc/mounts") - mounts += mountfh.read_nonblock(1024) while true + begin + mounts << mountfh.read_nonblock(1024) + rescue Errno::EINTR + # Popped out to process an unrelated signal, not because we're done + end while true else # Otherwise we shell out and let cat do it for us mountfh = IO.popen("/bin/cat /proc/mounts") diff --git a/spec/unit/exec_test_helper b/spec/unit/exec_test_helper new file mode 100755 index 0000000..ac536fe --- /dev/null +++ b/spec/unit/exec_test_helper @@ -0,0 +1,45 @@ +#!/usr/bin/env ruby + +require 'optparse' +options = OptionParser.new +options.on("--kill-me-withi SIGNAL",/USR[12]/) { |signal| $kill_me = signal } +options.on("--called-by PID",Integer) { |parent| $pid = parent } +options.on("--repeat TIMES",Integer) { |val| $n = val } +options.on("--delay-start SECONDS",Float) { |val| $delay_start = val } +options.on("--delay-exit SECONDS", Float) { |val| $delay_exit = val } +options.on("--delay-each SECONDS",Float) { |val| $delay_each = val } +options.on("--delay-close SECONDS",Float) { |val| $delay_close = val } +options.on("--flush") { $flush = true } +options.on("--fork-badly") { $fork_badly = true } +options.on("--write-in-fork") { $write_in_fork = true } +options.on("--signal-exit") { $signal_exit = true } +rest = options.parse(ARGV) + +unless rest.empty? + put options + exit +end + +unless ps_of_pid = `ps`.split(/\n/).grep(/^ *#{$pid}\b/).first and ps_of_pid =~ /ruby/ + fail "I don't believe you, #{$pid} is #{ps_of_pid||'not yours'}." +end + +sleep $delay_start if $delay_start +$n.times do + Process.kill $kill_me.to_sym,$pid if $kill_me + `sleep #{$delay_each || 0}` # Note that we want a process sleep, not a thread sleep + puts $pid + $stdout.flush if $flush +end + +unless $fork_badly and fork + $exit_signal = :USR2 # Something unexpected happened + trap(:PIPE) { $exit_signal = :PIPE } + at_exit { Process.kill $exit_signal,$pid} if $signal_exit + sleep $delay_close || 0 + puts 'Thanks for waiting' if $write_in_fork + $stdout.close + sleep $delay_exit || 0 + $exit_signal = :USR1 # We made it to the end! +end + diff --git a/spec/unit/util.rb b/spec/unit/util.rb new file mode 100755 index 0000000..b178e19 --- /dev/null +++ b/spec/unit/util.rb @@ -0,0 +1,177 @@ +#!/usr/bin/env ruby + +require File.dirname(__FILE__) + '/../spec_helper' + +describe Puppet::Util do + + describe "execute" do + it "should be a module method of Puppet::Util" do + Puppet::Util.should respond_to(:execute) + end + + # def execute(command, arguments = {:failonfail => true, :combine => true}) + + it "should insist that the command be an array" do + lambda { Puppet::Util.execute('test') }.should raise_error(ArgumentError) + end + + [false,nil,'not given'].each { |squelch| + describe "when squelch is #{squelch||squelch.inspect}" do + before :each do + @args = {} + @args[:squelch] = squelch unless squelch == 'not given' + end + it "should execute its command and return the results" do + Puppet::Util.execute(['echo','mydata'],@args).should == "mydata\n" + end + it "should be able to handle large amounts (>4kb) of output" do + # Ticket #2997 + Puppet::Util.execute(['cat',__FILE__],@args).should == File.read(__FILE__) + end + describe "and failonfail is false" do + it "should capture the error message on failure" do + # Ticket #2731, comment #11 + Puppet::Util.execute(["does_not_exist"],@args.update( :failonfail => false) ).should == "No such file or directory - does_not_exist\n" + end + end + describe "and failonfail is true" do + it "should capture the error message on failure" do + lambda {Puppet::Util.execute(["does_not_exist"],@args.update( :failonfail => true) )}.should raise_error + end + end + end + } + [true,'some non-nil value'].each { |squelch| + describe "when squelch is #{squelch}" do + it "should execute its command" do + f = Tempfile.new('execute_test') + Puppet::Util.execute(['rm',f.path],:squelch => squelch) + File.exists?(f.path).should == false + end + it "should ignore the results" do + Puppet::Util.execute(['echo','mydata'],:squelch => squelch).should be_nil + end + it "should be able to ignore large amounts (>4kb) of output" do + # Counterpart to ticket #2997 + Puppet::Util.execute(['cat',__FILE__],:squelch => squelch).should be_nil + end + describe "and failonfail is false" do + it "should not capture the error message on failure" do + Puppet::Util.execute(["does_not_exist"],:squelch => squelch, :failonfail => false).should be_nil + end + end + describe "and failonfail is true" do + it "should not capture the error message on failure" do + lambda {Puppet::Util.execute(["does_not_exist"],:squelch => squelch, :failonfail => true)}.should raise_error + end + end + end + } + [(:available if $stdin.respond_to? :readpartial),:unavailable].compact.each { |readpartial| + # + # Available/unavailable dichotomy motivated by Ticket #3013 + # Note that we can test how it would work on an old ruby + # when we're on a modern one but not the other way around. + # + before :each do + a,b = IO.pipe + if readpartial == :unavailable and a.respond_to? :readpartial + # + # We are on a modern ruby but want a to look like + # a pipe from 1.8.1/1.8.2 for this test + # + def a.respond_to?(x) + (x.to_sym != :readpartial) and super + end + def a.readpartial(*args) + method_missing(:readpartial,*args) + end + end + IO.stubs(:pipe).returns([a,b]) + end + describe "when readpartial is #{readpartial}" do + it "should be resilient to asynchronous signals" do + n = 200 + begin + usr1_count = 0 + old_usr1_trap = trap(:USR1) { usr1_count += 1 } + # + # Three components of resiliency are tested together here rather + # than in three separate it-blocks because the test is slow + # and combining them makes better use of our testing-time + # budget. + # + task = [File.dirname(__FILE__)+'/exec_test_helper','--kill-me-with','USR1','--called-by',$PID,'--repeat',n] + Puppet::Util.execute(task).should == "#{$PID}\n"*n + usr1_count.should <= n # We don't expect phantom signals + usr1_count.should >= n-n/4 # We expect 75% or better signal delivery + ensure + trap(:USR1,old_usr1_trap) + end + end + it "should not miss output if the other end doles it out bit at a time" do + n = 5 + task = [File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--repeat',n,'--flush','--delay-each',1.5] + Puppet::Util.execute(task).should == "#{$PID}\n"*n + end + it "should not mind (hang, crash, lose output) if the other end closes the pipe before completing" do + n = 5 + task = [File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--repeat',n,'--delay-exit',5] + timeout(10) { Puppet::Util.execute(task).should == "#{$PID}\n"*n } + end + it "should not hang if the child process terminates but does not close the pipe" do + # Ticket #1563 comment #7 + n = 5 + task = [File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',50] + timeout(20) { Puppet::Util.execute(task).should == "#{$PID}\n"*n } + end + describe "when the child process passes the pipe to a grandchild and terminates without closing the pipe or waiting for the grandchild" do + it "should not cause the grandchild child process to receive a SIGPIPE if the grandchild subsequently ignores the pipe" do + # Ticket #1563 comment #7, null hypothesis for Ticket #3013 comment #18 + n = 5 + task = [File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',30,'--signal-exit'] + # Note that the trapping setup/teardown appaently can NOT be done in a before block + signal = nil + old_usr1_trap = trap(:USR1) { signal = :USR1 } # They exited normally + old_usr2_trap = trap(:USR2) { signal = :USR2 } # They died + old_pipe_trap = trap(:PIPE) { signal = :PIPE } # They got a SIGPIPE + timeout(50) { + Puppet::Util.execute(task).should == "#{$PID}\n"*n + `sleep 0` until signal # Process sleep, not thread sleep + } + trap(:USR1,old_usr1_trap) + trap(:USR2,old_usr2_trap) + trap(:PIPE,old_pipe_trap) + signal.should == :USR1 + end + it "should not cause the grandchild child process to receive a SIGPIPE if the grandchild subsequently writes to the pipe" do + # Ticket #3013 comment #18 + pending %q{ + This situation is presently only a speculation and may in any case exceed + what we should expect the code to do; see note in the implementation for a + suggested way to handle it if needed. + } + n = 5 + task = [File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',30,'--write-in-fork','--signal-exit'] + # Note that the trapping setup/teardown apparently can NOT be done in a before block + signal = nil + old_usr1_trap = trap(:USR1) { signal = :USR1 } # They exited normally + old_usr2_trap = trap(:USR2) { signal = :USR2 } # They died + old_pipe_trap = trap(:PIPE) { signal = :PIPE } # They got a SIGPIPE + timeout(50) { + Puppet::Util.execute(task).should == "#{$PID}\n"*n + `sleep 0` until signal # Process sleep, not thread sleep + } + trap(:USR1,old_usr1_trap) + trap(:USR2,old_usr2_trap) + trap(:PIPE,old_pipe_trap) + signal.should == :USR1 + end + it "should be able to deal with the command-line 'pseudo-daemon&' idiom (but not retain the output)" do + Puppet::Util.execute(['/bin/sh','-c','(sleep 5; echo foo)&']).should == '' + end + end + end + } + end +end -- 1.6.4
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
