From 5c13830161279320b26efa5889175c28a8e65b3c Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Fri, 27 Jan 2017 13:20:57 -0800 Subject: remove largely useless Utils class its just a collection of private methods, they should be private methods retain a method in the class that was being used externally. Signed-off-by: Lamont Granquist --- .travis.yml | 1 + Rakefile | 2 +- appveyor.yml | 1 + lib/mixlib/shellout/windows.rb | 246 ++++++++++++++++++----------------- spec/mixlib/shellout/windows_spec.rb | 21 ++- spec/mixlib/shellout_spec.rb | 2 +- 6 files changed, 140 insertions(+), 133 deletions(-) diff --git a/.travis.yml b/.travis.yml index f77d573..28638b9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,5 +10,6 @@ branches: - master before_install: - gem update bundler + - gem update --system script: bundle exec rake diff --git a/Rakefile b/Rakefile index 03c1c26..06a7620 100644 --- a/Rakefile +++ b/Rakefile @@ -13,4 +13,4 @@ RSpec::Core::RakeTask.new(:spec) do |t| t.pattern = FileList["spec/**/*_spec.rb"] end -task default: [:style, :spec] +task default: [:spec, :style] diff --git a/appveyor.yml b/appveyor.yml index e49c59e..2276bde 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -22,6 +22,7 @@ install: - echo %PATH% - ruby --version - gem --version + - gem update --system - gem install bundler --quiet --no-ri --no-rdoc - bundler --version diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb index 0c66c2e..094e9b3 100644 --- a/lib/mixlib/shellout/windows.rb +++ b/lib/mixlib/shellout/windows.rb @@ -78,7 +78,7 @@ module Mixlib # Start the process # process = Process.create(create_process_args) - logger.debug(Utils.format_process(process, app_name, command_line, timeout)) if logger + logger.debug(format_process(process, app_name, command_line, timeout)) if logger begin # Start pushing data into input stdin_write << input if input @@ -109,7 +109,7 @@ module Mixlib begin require "wmi-lite/wmi" wmi = WmiLite::Wmi.new - Utils.kill_process_tree(process.process_id, wmi, logger) + kill_process_tree(process.process_id, wmi, logger) Process.kill(:KILL, process.process_id) rescue Errno::EIO, SystemCallError logger.warn("Failed to kill timed out process #{process.process_id}") if logger @@ -118,7 +118,7 @@ module Mixlib raise Mixlib::ShellOut::CommandTimeout, [ "command timed out:", format_for_exception, - Utils.format_process(process, app_name, command_line, timeout), + format_process(process, app_name, command_line, timeout), ].join("\n") end @@ -146,8 +146,6 @@ module Mixlib end end - private - class ThingThatLooksSortOfLikeAProcessStatus attr_accessor :exitstatus def success? @@ -155,6 +153,8 @@ module Mixlib end end + private + def consume_output(open_streams, stdout_read, stderr_read) return false if open_streams.length == 0 ready = IO.select(open_streams, nil, nil, READ_WAIT_TIME) @@ -188,7 +188,7 @@ module Mixlib IS_BATCH_FILE = /\.bat"?$|\.cmd"?$/i def command_to_run(command) - return _run_under_cmd(command) if Utils.should_run_under_cmd?(command) + return _run_under_cmd(command) if should_run_under_cmd?(command) candidate = candidate_executable_for_command(command) @@ -196,8 +196,8 @@ module Mixlib return [ nil, command ] if candidate.length == 0 # Check if the exe exists directly. Otherwise, search PATH. - exe = Utils.find_executable(candidate) - exe = Utils.which(unquoted_executable_path(command)) if exe.nil? && exe !~ /[\\\/]/ + exe = find_executable(candidate) + exe = which(unquoted_executable_path(command)) if exe.nil? && exe !~ /[\\\/]/ # Batch files MUST use cmd; and if we couldn't find the command we're looking for, # we assume it must be a cmd builtin. @@ -240,7 +240,7 @@ module Mixlib end environment.each_pair do |k, v| - if v == nil + if v.nil? result.delete(k) else result[k] = v @@ -249,134 +249,140 @@ module Mixlib result end - module Utils - # api: semi-private - # If there are special characters parsable by cmd.exe (such as file redirection), then - # this method should return true. - # - # This parser is based on - # https://github.com/ruby/ruby/blob/9073db5cb1d3173aff62be5b48d00f0fb2890991/win32/win32.c#L1437 - def self.should_run_under_cmd?(command) - return true if command =~ /^@/ - - quote = nil - env = false - env_first_char = false - - command.dup.each_char do |c| - case c - when "'", '"' - if !quote - quote = c - elsif quote == c - quote = nil - end - next - when ">", "<", "|", "&", "\n" - return true unless quote - when "%" - return true if env - env = env_first_char = true - next - else - next unless env - if env_first_char - env_first_char = false - (env = false) && next if c !~ /[A-Za-z_]/ - end - env = false if c !~ /[A-Za-z1-9_]/ + # api: semi-private + # If there are special characters parsable by cmd.exe (such as file redirection), then + # this method should return true. + # + # This parser is based on + # https://github.com/ruby/ruby/blob/9073db5cb1d3173aff62be5b48d00f0fb2890991/win32/win32.c#L1437 + def should_run_under_cmd?(command) + return true if command =~ /^@/ + + quote = nil + env = false + env_first_char = false + + command.dup.each_char do |c| + case c + when "'", '"' + if !quote + quote = c + elsif quote == c + quote = nil end + next + when ">", "<", "|", "&", "\n" + return true unless quote + when "%" + return true if env + env = env_first_char = true + next + else + next unless env + if env_first_char + env_first_char = false + (env = false) && next if c !~ /[A-Za-z_]/ + end + env = false if c !~ /[A-Za-z1-9_]/ end - return false end + return false + end - def self.pathext - @pathext ||= ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""] - end + def pathext + @pathext ||= ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""] + end - # which() mimicks the Unix which command - # FIXME: it is not working - def self.which(cmd) - ENV["PATH"].split(File::PATH_SEPARATOR).each do |path| - exe = find_executable("#{path}/#{cmd}") - return exe if exe - end - return nil + # which() mimicks the Unix which command + # FIXME: it is not working + def which(cmd) + ENV["PATH"].split(File::PATH_SEPARATOR).each do |path| + exe = find_executable("#{path}/#{cmd}") + return exe if exe end + return nil + end - # Windows has a different notion of what "executable" means - # The OS will search through valid the extensions and look - # for a binary there. - def self.find_executable(path) - return path if executable? path - - pathext.each do |ext| - exe = "#{path}#{ext}" - return exe if executable? exe - end - return nil - end + # Windows has a different notion of what "executable" means + # The OS will search through valid the extensions and look + # for a binary there. + def find_executable(path) + return path if executable? path - def self.executable?(path) - File.executable?(path) && !File.directory?(path) + pathext.each do |ext| + exe = "#{path}#{ext}" + return exe if executable? exe end + return nil + end - def self.system_required_processes - [ - "System Idle Process", - "System", - "spoolsv.exe", - "lsass.exe", - "csrss.exe", - "smss.exe", - "svchost.exe", - ] - end + def executable?(path) + File.executable?(path) && !File.directory?(path) + end - def self.unsafe_process?(name, logger) - return false unless system_required_processes.include? name - logger.debug( - "A request to kill a critical system process - #{name} - was received and skipped." - ) - true - end + def system_required_processes + [ + "System Idle Process", + "System", + "spoolsv.exe", + "lsass.exe", + "csrss.exe", + "smss.exe", + "svchost.exe", + ] + end - # recursively kills all child processes of given pid - # calls itself querying for children child procs until - # none remain. Important that a single WmiLite instance - # is passed in since each creates its own WMI rpc process - def self.kill_process_tree(pid, wmi, logger) - wmi.query("select * from Win32_Process where ParentProcessID=#{pid}").each do |instance| - next if unsafe_process?(instance.wmi_ole_object.name, logger) - child_pid = instance.wmi_ole_object.processid - kill_process_tree(child_pid, wmi, logger) - kill_process(instance, logger) - end - end + def unsafe_process?(name, logger) + return false unless system_required_processes.include? name + logger.debug( + "A request to kill a critical system process - #{name} - was received and skipped." + ) + true + end - def self.kill_process(instance, logger) + # recursively kills all child processes of given pid + # calls itself querying for children child procs until + # none remain. Important that a single WmiLite instance + # is passed in since each creates its own WMI rpc process + def kill_process_tree(pid, wmi, logger) + wmi.query("select * from Win32_Process where ParentProcessID=#{pid}").each do |instance| + next if unsafe_process?(instance.wmi_ole_object.name, logger) child_pid = instance.wmi_ole_object.processid - logger.debug([ - "killing child process #{child_pid}::", - "#{instance.wmi_ole_object.Name} of parent #{pid}", - ].join) if logger - Process.kill(:KILL, instance.wmi_ole_object.processid) - rescue Errno::EIO, SystemCallError - logger.debug([ - "Failed to kill child process #{child_pid}::", - "#{instance.wmi_ole_object.Name} of parent #{pid}", - ].join) if logger + kill_process_tree(child_pid, wmi, logger) + kill_process(instance, logger) end + end + + def kill_process(instance, logger) + child_pid = instance.wmi_ole_object.processid + logger.debug([ + "killing child process #{child_pid}::", + "#{instance.wmi_ole_object.Name} of parent #{pid}", + ].join) if logger + Process.kill(:KILL, instance.wmi_ole_object.processid) + rescue Errno::EIO, SystemCallError + logger.debug([ + "Failed to kill child process #{child_pid}::", + "#{instance.wmi_ole_object.Name} of parent #{pid}", + ].join) if logger + end + + def format_process(process, app_name, command_line, timeout) + msg = [] + msg << "ProcessId: #{process.process_id}" + msg << "app_name: #{app_name}" + msg << "command_line: #{command_line}" + msg << "timeout: #{timeout}" + msg.join("\n") + end - def self.format_process(process, app_name, command_line, timeout) - msg = [] - msg << "ProcessId: #{process.process_id}" - msg << "app_name: #{app_name}" - msg << "command_line: #{command_line}" - msg << "timeout: #{timeout}" - msg.join("\n") + # DEPRECATED do not use + class Utils + include Mixlib::ShellOut::Windows + def self.should_run_under_cmd?(cmd) + Mixlib::ShellOut::Windows::Utils.new.send(:should_run_under_cmd?, cmd) end end - end # class + end end end diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb index fe511eb..cd8ab04 100644 --- a/spec/mixlib/shellout/windows_spec.rb +++ b/spec/mixlib/shellout/windows_spec.rb @@ -4,7 +4,7 @@ describe "Mixlib::ShellOut::Windows", :windows_only do describe "Utils" do describe ".should_run_under_cmd?" do - subject { Mixlib::ShellOut::Windows::Utils.should_run_under_cmd?(command) } + subject { Mixlib::ShellOut.new.send(:should_run_under_cmd?, command) } def self.with_command(_command, &example) context "with command: #{_command}" do @@ -110,7 +110,7 @@ describe "Mixlib::ShellOut::Windows", :windows_only do end describe ".kill_process_tree" do - let(:utils) { Mixlib::ShellOut::Windows::Utils } + let(:shell_out) { Mixlib::ShellOut.new } let(:wmi) { Object.new } let(:wmi_ole_object) { Object.new } let(:wmi_process) { Object.new } @@ -129,8 +129,8 @@ describe "Mixlib::ShellOut::Windows", :windows_only do end it "does not attempt to kill csrss.exe" do - expect(utils).to_not receive(:kill_process) - utils.kill_process_tree(200, wmi, logger) + expect(shell_out).to_not receive(:kill_process) + shell_out.send(:kill_process_tree, 200, wmi, logger) end end @@ -141,10 +141,10 @@ describe "Mixlib::ShellOut::Windows", :windows_only do end it "does attempt to kill blah.exe" do - expect(utils).to receive(:kill_process).with(wmi_process, logger) - expect(utils).to receive(:kill_process_tree).with(200, wmi, logger).and_call_original - expect(utils).to receive(:kill_process_tree).with(300, wmi, logger) - utils.kill_process_tree(200, wmi, logger) + expect(shell_out).to receive(:kill_process).with(wmi_process, logger) + expect(shell_out).to receive(:kill_process_tree).with(200, wmi, logger).and_call_original + expect(shell_out).to receive(:kill_process_tree).with(300, wmi, logger) + shell_out.send(:kill_process_tree, 200, wmi, logger) end end end @@ -186,9 +186,8 @@ describe "Mixlib::ShellOut::Windows", :windows_only do let(:stubbed_shell_out) { raise NotImplemented, "Must declare let(:stubbed_shell_out)" } let(:shell_out) { Mixlib::ShellOut.new(cmd) } - let(:utils) { Mixlib::ShellOut::Windows::Utils } - let(:with_valid_exe_at_location) { lambda { |s| allow(utils).to receive(:find_executable).and_return(executable_path) } } - let(:with_invalid_exe_at_location) { lambda { |s| allow(utils).to receive(:find_executable).and_return(nil) } } + let(:with_valid_exe_at_location) { lambda { |s| allow(shell_out).to receive(:find_executable).and_return(executable_path) } } + let(:with_invalid_exe_at_location) { lambda { |s| allow(shell_out).to receive(:find_executable).and_return(nil) } } context "with empty command" do let(:stubbed_shell_out) { shell_out } diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb index b5df5dd..a55b3c0 100644 --- a/spec/mixlib/shellout_spec.rb +++ b/spec/mixlib/shellout_spec.rb @@ -1130,7 +1130,7 @@ describe Mixlib::ShellOut do context "and child processes should be killed" do it "kills the child processes" do - expect(Mixlib::ShellOut::Windows::Utils).to receive(:kill_process) do |instance| + expect(shell_cmd).to receive(:kill_process) do |instance| expect(instance.wmi_ole_object.Name).to match(/powershell/) Process.kill(:KILL, instance.wmi_ole_object.processid) end -- cgit v1.2.1