Merge pull request #127 from chef/smurawski/agressive_termination

Keep Mixlib-ShellOut from killing required Windows processes

3 files changed, 81 insertions, 16 deletions

diff --git a/Gemfile b/Gemfile
index d080412..1f468d9 100644
--- a/Gemfile
+++ b/Gemfile
@@ -9,4 +9,7 @@ end
 
 group(:development) do
   gem 'pry'
+  gem 'pry-byebug'
+  gem 'pry-stack_explorer'
+  gem 'rb-readline'
 end
diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb
index fd8039b..5b13dad 100644
--- a/lib/mixlib/shellout/windows.rb
+++ b/lib/mixlib/shellout/windows.rb
@@ -321,31 +321,51 @@ module Mixlib
           File.executable?(path) && !File.directory?(path)
         end
 
+        def self.system_required_processes
+          [
+            'System Idle Process',
+            'System',
+            'spoolsv.exe',
+            'lsass.exe',
+            'csrss.exe',
+            'smss.exe',
+            'svchost.exe'
+          ]
+        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
+
         # 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)
-            begin
-              logger.debug([
-                "killing child process #{child_pid}::",
-                "#{instance.wmi_ole_object.Name} of parent #{pid}"
-                ].join) if logger
-              kill_process(instance)
-            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
+            kill_process(instance, logger)
           end
         end
 
-        def self.kill_process(instance)
+        def self.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 self.format_process(process, app_name, command_line, timeout)
diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb
index dd8a80b..8b40fde 100644
--- a/spec/mixlib/shellout/windows_spec.rb
+++ b/spec/mixlib/shellout/windows_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 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) }
@@ -108,6 +108,46 @@ describe 'Mixlib::ShellOut::Windows', :windows_only do
         end
       end
     end
+
+    describe '.kill_process_tree' do
+      let(:utils) { Mixlib::ShellOut::Windows::Utils }
+      let(:wmi) { Object.new }
+      let(:wmi_ole_object) { Object.new }
+      let(:wmi_process) { Object.new }
+      let(:logger) { Object.new }
+
+      before do
+        allow(wmi).to receive(:query).and_return([wmi_process])
+        allow(wmi_process).to receive(:wmi_ole_object).and_return(wmi_ole_object)
+        allow(logger).to receive(:debug)
+      end
+
+      context 'with a protected system process in the process tree' do
+        before do
+          allow(wmi_ole_object).to receive(:name).and_return('csrss.exe')
+          allow(wmi_ole_object).to receive(:processid).and_return(100)
+        end
+
+        it 'does not attempt to kill csrss.exe' do
+          expect(utils).to_not receive(:kill_process)
+          utils.kill_process_tree(200, wmi, logger)
+        end
+      end
+
+      context 'with a non-system-critical process in the process tree' do
+        before do
+          allow(wmi_ole_object).to receive(:name).and_return('blah.exe')
+          allow(wmi_ole_object).to receive(:processid).and_return(300)
+        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)
+        end
+      end
+    end
   end
 
   # Caveat: Private API methods are subject to change without notice.
@@ -170,7 +210,7 @@ describe 'Mixlib::ShellOut::Windows', :windows_only do
           allow(ENV).to receive(:[]).with('COMSPEC').and_return('C:\Windows\system32\cmd.exe')
           allow(File).to receive(:executable?).and_return(false)
           allow(File).to receive(:executable?).with(executable_path).and_return(true)
-          allow(File).to receive(:directory?).and_return(false)            
+          allow(File).to receive(:directory?).and_return(false)
         end
 
         it 'should return with full path with extension' do
@@ -181,7 +221,7 @@ describe 'Mixlib::ShellOut::Windows', :windows_only do
           before do
             # File.executable? returns true for directories
             allow(File).to receive(:executable?).with(cmd).and_return(true)
-            allow(File).to receive(:directory?).with(cmd).and_return(true)            
+            allow(File).to receive(:directory?).with(cmd).and_return(true)
           end
 
           it 'should return with full path with extension' do
@@ -196,6 +236,8 @@ describe 'Mixlib::ShellOut::Windows', :windows_only do
         let(:cmd_exe) { "C:\\Windows\\system32\\cmd.exe" }
         let(:cmd) { "#{executable_path}" }
 
+        before { ENV['ComSpec'] = 'C:\Windows\system32\cmd.exe' }
+
         context 'with .bat file' do
           let(:executable_path) { '"C:\Program Files\Application\Start.bat"' }