Merge pull request #182 from chef/lcg/win-array-args

Support array args on windows WIP

3 files changed, 127 insertions, 3 deletions

diff --git a/Rakefile b/Rakefile
index dd326ab..40ab182 100644
--- a/Rakefile
+++ b/Rakefile
@@ -3,7 +3,7 @@ require "rspec/core/rake_task"
 
 Bundler::GemHelper.install_tasks name: "mixlib-shellout"
 
-task default: [:style, :spec]
+task default: [:spec, :style]
 
 desc "Run specs"
 RSpec::Core::RakeTask.new(:spec) do |spec|
diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb
index 27adbbd..cde795b 100644
--- a/lib/mixlib/shellout/windows.rb
+++ b/lib/mixlib/shellout/windows.rb
@@ -2,7 +2,7 @@
 # Author:: Daniel DeLeo (<dan@chef.io>)
 # Author:: John Keiser (<jkeiser@chef.io>)
 # Author:: Ho-Sheng Hsiao (<hosh@chef.io>)
-# Copyright:: Copyright (c) 2011-2016 Chef Software, Inc.
+# Copyright:: Copyright (c) 2011-2019, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -66,7 +66,7 @@ module Mixlib
           #
           # Set cwd, environment, appname, etc.
           #
-          app_name, command_line = command_to_run(command)
+          app_name, command_line = command_to_run(combine_args(*command))
           create_process_args = {
             app_name: app_name,
             command_line: command_line,
@@ -196,6 +196,46 @@ module Mixlib
         true
       end
 
+      # Use to support array passing semantics on windows
+      #
+      # 1.  strings with whitespace or quotes in them need quotes around them.
+      # 2.  interior quotes need to get backslash escaped (parser needs to know when it really ends).
+      # 3.  random backlsashes in paths themselves remain untouched.
+      # 4.  if the argument must be quoted by #1 and terminates in a sequence of backslashes then all the backlashes must themselves
+      #     be backslash excaped (double the backslashes).
+      # 5.  if an interior quote that must be escaped by #2 has a sequence of backslashes before it then all the backslashes must
+      #     themselves be backslash excaped along with the backslash ecape of the interior quote (double plus one backslashes).
+      #
+      # And to restate.  We are constructing a string which will be parsed by the windows parser into arguments, and we want those
+      # arguments to match the *args array we are passed here.  So call the windows parser operation A then we need to apply A^-1 to
+      # our args to construct the string so that applying A gives windows back our *args.
+      #
+      # And when the windows parser sees a series of backslashes followed by a double quote, it has to determine if that double quote
+      # is terminating or not, and how many backslashes to insert in the args.  So what it does is divide it by two (rounding down) to
+      # get the number of backslashes to insert.  Then if it is even the double quotes terminate the argument.  If it is even the
+      # double quotes are interior double quotes (the extra backslash quotes the double quote).
+      #
+      # We construct the inverse operation so interior double quotes preceeded by N backslashes get 2N+1 backslashes in front of the quote,
+      # while trailing N backslashes get 2N backslashes in front of the quote that terminates the argument.
+      #
+      # see: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
+      #
+      # @api private
+      # @param args [Array<String>] array of command arguments
+      # @return String
+      def combine_args(*args)
+        return args[0] if args.length == 1
+        args.map do |arg|
+          if arg =~ /[ \t\n\v"]/
+            arg = arg.gsub(/(\\*)"/, '\1\1\"') # interior quotes with N preceeding backslashes need 2N+1 backslashes
+            arg = arg.sub(/(\\+)$/, '\1\1') # trailing N backslashes need to become 2N backslashes
+            "\"#{arg}\""
+          else
+            arg
+          end
+        end.join(" ")
+      end
+
       def command_to_run(command)
         return run_under_cmd(command) if should_run_under_cmd?(command)
 
diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb
index d4edabc..1011560 100644
--- a/spec/mixlib/shellout/windows_spec.rb
+++ b/spec/mixlib/shellout/windows_spec.rb
@@ -307,4 +307,88 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
       end
     end
   end
+
+  context "#combine_args" do
+    let(:shell_out) { Mixlib::ShellOut.new }
+    subject { shell_out.send(:combine_args, *largs) }
+
+    def self.with_args(*args, &example)
+      context "with command #{args}" do
+        let(:largs) { args }
+        it(&example)
+      end
+    end
+
+    with_args("echo", "%PATH%") do
+      is_expected.to eql(%q{echo %PATH%})
+    end
+
+    with_args("echo %PATH%") do
+      is_expected.to eql(%q{echo %PATH%})
+    end
+
+    # Note carefully for the following that single quotes in ruby support '\\' as an escape sequence for a single
+    # literal backslash.  It is not mandatory to always use this since '\d' does not escape the 'd' and is literally
+    # a backlash followed by an 'd'.  However, in the following all backslashes are escaped for consistency.  Otherwise
+    # it becomes prohibitively confusing to track when you need and do not need the escape the backslash (particularly
+    # when the literal string has a trailing backslash such that '\\' must be used instead of '\' which would escape
+    # the intended terminating single quote or %q{\} which escapes the terminating delimiter).
+
+    with_args("child.exe", "argument1", "argument 2", '\\some\\path with\\spaces') do
+      is_expected.to eql('child.exe argument1 "argument 2" "\\some\\path with\\spaces"')
+    end
+
+    with_args("child.exe", "argument1", 'she said, "you had me at hello"', '\\some\\path with\\spaces') do
+      is_expected.to eql('child.exe argument1 "she said, \\"you had me at hello\\"" "\\some\\path with\\spaces"')
+    end
+
+    with_args("child.exe", "argument1", 'argument\\\\"2\\\\"', "argument3", "argument4") do
+      is_expected.to eql('child.exe argument1 "argument\\\\\\\\\\"2\\\\\\\\\\"" argument3 argument4')
+    end
+
+    with_args("child.exe", '\\some\\directory with\\spaces\\', "argument2") do
+      is_expected.to eql('child.exe "\\some\\directory with\\spaces\\\\" argument2')
+    end
+
+    with_args("child.exe", '\\some\\directory with\\\\\\spaces\\\\\\', "argument2") do
+      is_expected.to eql('child.exe "\\some\\directory with\\\\\\spaces\\\\\\\\\\\\" argument2')
+    end
+  end
+
+  context "#run_command" do
+    let(:shell_out) { Mixlib::ShellOut.new(*largs) }
+    subject { shell_out.send(:run_command) }
+
+    def self.with_args(*args, &example)
+      context "with command #{args}" do
+        let(:largs) { args }
+        it(&example)
+      end
+    end
+
+    with_args("echo", "FOO") do
+      is_expected.not_to be_error
+    end
+
+    # Test box is going to have to have c:\program files on it, which is perhaps brittle in principle, but
+    # I don't know enough windows to come up with a less brittle test.  Fix it if you've got a better idea.
+    # The tests need to fail though if the argument is not quoted correctly so using `echo` would be poor
+    # because `echo FOO BAR` and `echo "FOO BAR"` aren't any different.
+
+    with_args('dir c:\\program files') do
+      is_expected.to be_error
+    end
+
+    with_args('dir "c:\\program files"') do
+      is_expected.not_to be_error
+    end
+
+    with_args("dir", 'c:\\program files') do
+      is_expected.not_to be_error
+    end
+
+    with_args("dir", 'c:\\program files\\') do
+      is_expected.not_to be_error
+    end
+  end
 end