From 7f5fbf448c8dd572569221587343e074acacd556 Mon Sep 17 00:00:00 2001 From: Victor Robertson Date: Sat, 25 Jan 2014 14:32:20 -0600 Subject: [PATCH] Process SystemCommand arguments in list format Previously, SystemCommand commands and arguments were processed as strings which caused problems during shell interpretation if the arguments were not escaped properly. Now all commands are expressed as arrays and no longer require their arguments to be escaped. Additionally, stderr and stdout could have been interleaved in the past and now they are always separated. --- lib/cask/artifact/pkg.rb | 4 +-- lib/cask/pkg.rb | 2 +- lib/cask/system_command.rb | 48 +++++++++++-------------- test/cask/artifact/pkg_test.rb | 54 ++++++++++++++--------------- test/cask/cli/alfred_test.rb | 8 ++--- test/cask/container/naked_test.rb | 2 +- test/cask/pkg_test.rb | 6 ++-- test/support/fake_system_command.rb | 2 +- 8 files changed, 60 insertions(+), 66 deletions(-) diff --git a/lib/cask/artifact/pkg.rb b/lib/cask/artifact/pkg.rb index bbf02806f4..69ec9f1b4d 100644 --- a/lib/cask/artifact/pkg.rb +++ b/lib/cask/artifact/pkg.rb @@ -84,9 +84,9 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base if uninstall_options.key? :quit [*uninstall_options[:quit]].each do |id| ohai "Quitting application ID #{id}" - num_running = @command.run!('/usr/bin/osascript', :args => ['-e', "tell application \"System Events\" to count processes whose bundle identifier is \"#{id}\""], :sudo => true).to_i + num_running = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to count processes whose bundle identifier is "#{id}"}], :sudo => true).to_i if num_running > 0 - @command.run!('/usr/bin/osascript', :args => ['-e', "tell application id \"#{id}\" to quit"], :sudo => true) + @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application id "#{id}" to quit}], :sudo => true) end end end diff --git a/lib/cask/pkg.rb b/lib/cask/pkg.rb index 6425957c5c..95a750a26f 100644 --- a/lib/cask/pkg.rb +++ b/lib/cask/pkg.rb @@ -1,6 +1,6 @@ class Cask::Pkg def self.all_matching(regexp, command) - command.run(%Q(/usr/sbin/pkgutil --pkgs="#{regexp}")).split("\n").map do |package_id| + command.run('/usr/sbin/pkgutil', :args => [%Q{--pkgs=#{regexp}}]).split("\n").map do |package_id| new(package_id.chomp, command) end end diff --git a/lib/cask/system_command.rb b/lib/cask/system_command.rb index 33292b52f7..024673615d 100644 --- a/lib/cask/system_command.rb +++ b/lib/cask/system_command.rb @@ -1,14 +1,21 @@ +require 'open3' + class Cask::SystemCommand - def self.run(command, options={}) - command = _process_options(command, options) - odebug "Executing: #{command}" + def self.run(executable, options={}) + command = _process_options(executable, options) + odebug "Executing: #{command.inspect}" output = '' - IO.popen(command, 'r+') do |pipe| + Open3.popen3(*command) do |stdin, stdout, stderr| if options[:input] - options[:input].each { |line| pipe.puts line } + options[:input].each { |line| stdin.puts line } end - pipe.close_write - while line = pipe.gets + stdin.close_write + while line = stdout.gets + output << line + ohai line.chomp if options[:print] + end + while line = stderr.gets + next if options[:stderr] == :silence output << line ohai line.chomp if options[:print] end @@ -25,33 +32,20 @@ class Cask::SystemCommand run(command, options.merge(:must_succeed => true)) end - def self._process_options(command, options) + def self._process_options(executable, options) + command = [executable] if options[:sudo] - command = "/usr/bin/sudo -E -- #{_quote(command)}" + command.unshift('/usr/bin/sudo', '-E', '--') end - if options[:args] - command = "#{command} #{options[:args].map { |arg| _quote(arg) }.join(' ')}" - end - case options[:stderr] - when :silence then - command = "#{command} 2>/dev/null" - when :merge, nil then - command = "#{command} 2>&1" + if ! options[:args].empty? + command.concat options[:args] end command end def self._assert_success(status, command, output) unless status.success? - raise CaskCommandFailedError.new(command, output) - end - end - - def self._quote(string) - if %r{^(['"]).*\1$}.match(string) - string - else - %Q('#{string}') + raise CaskCommandFailedError.new(command.inspect, output) end end @@ -62,7 +56,7 @@ class Cask::SystemCommand raise CaskError.new(<<-ERRMSG) Error parsing plist output from command. command was: - #{command} + #{command.inspect} output we attempted to parse: #{output} ERRMSG diff --git a/test/cask/artifact/pkg_test.rb b/test/cask/artifact/pkg_test.rb index c35c304963..128e02208b 100644 --- a/test/cask/artifact/pkg_test.rb +++ b/test/cask/artifact/pkg_test.rb @@ -12,14 +12,11 @@ describe Cask::Artifact::Pkg do it 'runs the system installer on the specified pkgs' do pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand) - expected_command = "/usr/bin/sudo -E -- '/usr/sbin/installer' '-pkg' '#{@cask.destination_path/'MyFancyPkg'/'Fancy.pkg'}' '-target' '/' 2>&1" - Cask::FakeSystemCommand.stubs_command(expected_command) + Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/installer', '-pkg', @cask.destination_path/'MyFancyPkg'/'Fancy.pkg', '-target', '/']) shutup do pkg.install end - - Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1 end end @@ -27,17 +24,14 @@ describe Cask::Artifact::Pkg do it 'runs the specified uninstaller for the cask' do pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand) - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"' 2>&1), '1') - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application id "my.fancy.package.app" to quit' 2>&1)) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"'], '1') + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application id "my.fancy.package.app" to quit']) - expected_command = "/usr/bin/sudo -E -- '#{@cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool'}' '--please' 2>&1" - Cask::FakeSystemCommand.stubs_command(expected_command) + Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', @cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool', '--please']) shutup do pkg.uninstall end - - Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1 end it 'can uninstall using pkgutil, launchctl, and file lists' do @@ -45,7 +39,7 @@ describe Cask::Artifact::Pkg do pkg = Cask::Artifact::Pkg.new(cask, Cask::FakeSystemCommand) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil --pkgs="my.fancy.package.*" 2>&1), + ['/usr/sbin/pkgutil', '--pkgs=my.fancy.package.*'], [ 'my.fancy.package.main', 'my.fancy.package.agent', @@ -53,14 +47,14 @@ describe Cask::Artifact::Pkg do ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.main' 2>&1), + ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.main'], [ 'fancy/bin/fancy.exe', 'fancy/var/fancy.data', ].join("\n") ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.main' 2>&1), + ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.main'], [ 'fancy', 'fancy/bin', @@ -68,7 +62,7 @@ describe Cask::Artifact::Pkg do ].join("\n") ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.main' 2>&1), + ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.main'], <<-PLIST @@ -84,11 +78,11 @@ describe Cask::Artifact::Pkg do ) Cask::FakeSystemCommand.stubs_command( - %Q(/bin/launchctl 'list' '-x' 'my.fancy.package.service' 2>&1), + ['/bin/launchctl', 'list', '-x', 'my.fancy.package.service'], "launchctl list returned unknown response\n" ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/bin/sudo -E -- '/bin/launchctl' 'list' '-x' 'my.fancy.package.service' 2>&1), + ['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'list', '-x', 'my.fancy.package.service'], <<-PLIST @@ -112,14 +106,15 @@ describe Cask::Artifact::Pkg do PLIST ) - Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/bin/launchctl' 'remove' '--' 'my.fancy.package.service' 2>&1)) - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/kextstat' '-l' '-b' 'my.fancy.package.kernelextension' 2>&1), 'loaded') - Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/sbin/kextunload' '-b' '--' 'my.fancy.package.kernelextension' 2>&1)) - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.main' 2>&1)) + Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'remove', '--', 'my.fancy.package.service']) + + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/kextstat', '-l', '-b', 'my.fancy.package.kernelextension'], 'loaded') + Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/sbin/kextunload', '-b', '--', 'my.fancy.package.kernelextension']) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.main']) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.agent' 2>&1), + ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.agent'], [ 'fancy/agent/fancy-agent.exe', 'fancy/agent/fancy-agent.pid', @@ -127,14 +122,14 @@ describe Cask::Artifact::Pkg do ].join("\n") ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.agent' 2>&1), + ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.agent'], [ 'fancy', 'fancy/agent', ].join("\n") ) Cask::FakeSystemCommand.stubs_command( - %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.agent' 2>&1), + ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.agent'], <<-PLIST @@ -155,13 +150,18 @@ describe Cask::Artifact::Pkg do /tmp/fancy/bin /tmp/fancy/var ].each do |dir| - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/chmod' '--' '777' '#{dir}' 2>&1)) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/chmod', '--', '777', '#{dir}']) end - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.agent' 2>&1)) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.agent']) - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/bin/fancy.exe' '/tmp/fancy/var/fancy.data' 2>&1)) - Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/agent/fancy-agent.exe' '/tmp/fancy/agent/fancy-agent.pid' '/tmp/fancy/agent/fancy-agent.log' 2>&1)) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--', + Pathname.new('/tmp/fancy/bin/fancy.exe'), + Pathname.new('/tmp/fancy/var/fancy.data')]) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--', + Pathname.new('/tmp/fancy/agent/fancy-agent.exe'), + Pathname.new('/tmp/fancy/agent/fancy-agent.pid'), + Pathname.new('/tmp/fancy/agent/fancy-agent.log')]) # No assertions after call since all assertions are implicit from the interactions setup above. # TODO: verify rmdir commands (requires setting up actual file tree or faking out .exists? diff --git a/test/cask/cli/alfred_test.rb b/test/cask/cli/alfred_test.rb index c72191e7c5..430d0d6007 100644 --- a/test/cask/cli/alfred_test.rb +++ b/test/cask/cli/alfred_test.rb @@ -1,7 +1,7 @@ require 'test_helper' def fake_alfred_preference(key, response) - Cask::FakeSystemCommand.stubs_command("/usr/bin/defaults 'read' 'com.runningwithcrayons.Alfred-Preferences' '#{key}' 2>&1", response) + Cask::FakeSystemCommand.stubs_command(['/usr/bin/defaults', 'read', 'com.runningwithcrayons.Alfred-Preferences', key], response) end def fake_alfred_installed(installed=true) @@ -78,7 +78,7 @@ describe Cask::CLI::Alfred do SCOPE_RESPONSE Cask::FakeSystemCommand.stubs_command( - %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')" 2>&1) + ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')"}] ) TestHelper.must_output(self, lambda { @@ -95,7 +95,7 @@ describe Cask::CLI::Alfred do expected_scopes = (Cask::CLI::Alfred::DEFAULT_SCOPES + [Cask.caskroom]).map { |s| "'#{s}'" } Cask::FakeSystemCommand.stubs_command( - %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "(#{expected_scopes.join(',')})" 2>&1) + ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"(#{expected_scopes.join(',')})"}] ) TestHelper.must_output(self, lambda { @@ -140,7 +140,7 @@ describe Cask::CLI::Alfred do SCOPE_RESPONSE Cask::FakeSystemCommand.stubs_command( - %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')" 2>&1) + ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')"}] ) TestHelper.must_output(self, lambda { diff --git a/test/cask/container/naked_test.rb b/test/cask/container/naked_test.rb index 11af3cf790..6a6dda34f0 100644 --- a/test/cask/container/naked_test.rb +++ b/test/cask/container/naked_test.rb @@ -11,7 +11,7 @@ describe Cask::Container::Naked do cask = SpaceyCask.new path = '/tmp/downloads/kevin-spacey-1.2.pkg' expected_destination = cask.destination_path.join('kevin spacey.pkg') - expected_command = %Q(/usr/bin/ditto '--' '#{path}' '#{expected_destination}' 2>&1) + expected_command = ['/usr/bin/ditto', '--', path, expected_destination] Cask::FakeSystemCommand.stubs_command(expected_command) container = Cask::Container::Naked.new(cask, path, Cask::FakeSystemCommand) diff --git a/test/cask/pkg_test.rb b/test/cask/pkg_test.rb index c4acee8300..e6238d1d13 100644 --- a/test/cask/pkg_test.rb +++ b/test/cask/pkg_test.rb @@ -23,14 +23,14 @@ describe Cask::Pkg do pkg = Cask::Pkg.new('my.fake.pkg', Cask::FakeSystemCommand) Cask::FakeSystemCommand.stubs_command( - "/usr/sbin/pkgutil '--only-files' '--files' 'my.fake.pkg' 2>&1" + ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fake.pkg'] ) Cask::FakeSystemCommand.stubs_command( - "/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fake.pkg' 2>&1" + ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fake.pkg'] ) Cask::FakeSystemCommand.expects_command( - %q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fake.pkg' 2>&1) + ['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fake.pkg'] ) pkg.uninstall diff --git a/test/support/fake_system_command.rb b/test/support/fake_system_command.rb index 4bf7c32cad..22381a42df 100644 --- a/test/support/fake_system_command.rb +++ b/test/support/fake_system_command.rb @@ -29,7 +29,7 @@ class Cask::FakeSystemCommand def self.verify_expectations! expectations.each do |command, times| unless system_calls[command] == times - fail("expected #{command} to be run #{times} times, but got #{system_calls[command]}") + fail("expected #{command.inspect} to be run #{times} times, but got #{system_calls[command]}") end end end