Please review pull request #189: (#13678) Expand commands to absolute paths before running them opened by (stschulte)
Description:
The former behaviour of facter is to allow passing commands to Facter::Util::Resolution.exec without specifying an absolute path to the binary . To check if the executable is present on the system facter would then run %x{which commandname}. If this worked it would then run %x{commandname} and let the shell find the command. As a result we end up with two shell invocations just to run a single command.
The patch now tries to convert the executable of the command into an absolute pathname so we can easiliy check with the File.executable? if the binary is present on our system.
The new methods which and absolute_path? are adopted from puppet which claim to run on both windows and unix systems. This is great because we do not have to distinct between the platforms in the exec method anymore.
The reason I started to work on this is because the ifconfig binary moved from /sbin to /bin. Instead of writing a bunch of conditionals it might be easier to let facter find the binary for us. But relying on the former which behaviour of facter will break the ipaddress facts for everyone with ifconfig in /sbin/ifconfig but not having /sbin in path. The new implemenation makes sure that /sbin and /bin are checked when searching for the binary.
- Opened: Sat Apr 07 11:46:15 UTC 2012
- Based on: puppetlabs:1.6.x (96256a35bb6143cf0299c98462d4139a6f731e82)
- Requested merge: stschulte:refactor/1.6.x/13678 (5be1ed29fc248e24a1eea26be0c9a481c8586d9f)
Diff follows:
diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index a5f65c3..b8454ec 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -13,18 +13,63 @@ class Facter::Util::Resolution
INTERPRETER = Facter::Util::Config.is_windows? ? "cmd.exe" : "/bin/sh"
- def self.have_which
- if ! defined?(@have_which) or @have_which.nil?
- if Facter.value(:kernel) == 'windows'
- @have_which = false
- else
- %x{which which >/dev/null 2>&1}
- @have_which = ($? == 0)
+ def self.search_paths
+ if Facter::Util::Config.is_windows?
+ ENV['PATH'].split(File::PATH_SEPARATOR)
+ else
+ # Make sure facter is usable even for non-root users. Most commands
+ # in /sbin (like ifconfig) can be run as non priviledged users as
+ # long as they do not modify anything - which we do not do with facter
+ ENV['PATH'].split(File::PATH_SEPARATOR) + [ '/sbin', '/usr/sbin' ]
+ end
+ end
+
+ # taken from puppet: lib/puppet/util.rb
+ def self.which(bin)
+ if absolute_path?(bin)
+ return bin if File.executable?(bin)
+ else
+ search_paths.each do |dir|
+ dest = File.join(dir, bin)
+ if Facter::Util::Config.is_windows? && File.extname(dest).empty?
+ exts = ENV['PATHEXT']
+ exts = exts ? exts.split(File::PATH_SEPARATOR) : %w[.COM .EXE .BAT .CMD]
+ exts.each do |ext|
+ destext = dest + ext
+ return destext if File.executable?(destext)
+ end
+ end
+ return dest if File.executable?(dest)
end
end
- @have_which
+ nil
+ end
+
+ # taken from puppet: lib/puppet/util.rb
+ def self.absolute_path?(path, platform=nil)
+ # Escape once for the string literal, and once for the regex.
+ slash = '[\\\\/]'
+ name = '[^\\\\/]+'
+ regexes = {
+ :windows => %r!^(([A-Z]:#{slash})|(#{slash}#{slash}#{name}#{slash}#{name})|(#{slash}#{slash}\?#{slash}#{name}))!i,
+ :posix => %r!^/!,
+ }
+ platform ||= Facter::Util::Config.is_windows? ? :windows : :posix
+
+ !! (path =~ regexes[platform])
end
+ def self.expand_command(command)
+ if match = /^"([^"]+)"(?:\s+(.*))?/.match(command)
+ exe, arguments = match.captures
+ exe = which(exe) and [ "\"#{exe}\"", arguments ].compact.join(" ")
+ else
+ exe, arguments = command.split(/ /,2)
+ exe = which(exe) and [ exe, arguments ].compact.join(" ")
+ end
+ end
+
+
# Execute a program and return the output of that program.
#
# Returns nil if the program can't be found, or if there is a problem
@@ -33,37 +78,12 @@ def self.have_which
def self.exec(code, interpreter = nil)
Facter.warnonce "The interpreter parameter to 'exec' is deprecated and will be removed in a future version." if interpreter
- # Try to guess whether the specified code can be executed by looking at the
- # first word. If it cannot be found on the PATH defer on resolving the fact
- # by returning nil.
- # This only fails on shell built-ins, most of which are masked by stuff in
- # /bin or of dubious value anyways. In the worst case, "sh -c 'builtin'" can
- # be used to work around this limitation
- #
- # Windows' %x{} throws Errno::ENOENT when the command is not found, so we
- # can skip the check there. This is good, since builtins cannot be found
- # elsewhere.
- if have_which and !Facter::Util::Config.is_windows?
- path = nil
- binary = code.split.first
- if code =~ /^\//
- path = binary
- else
- path = %x{which #{binary} 2>/dev/null}.chomp
- # we don't have the binary necessary
- return nil if path == "" or path.match(/Command not found\./)
- end
-
- return nil unless FileTest.exists?(path)
- end
+ return nil unless code = expand_command(code)
out = nil
begin
out = %x{#{code}}.chomp
- rescue Errno::ENOENT => detail
- # command not found on Windows
- return nil
rescue => detail
$stderr.puts detail
return nil
diff --git a/spec/unit/util/resolution_spec.rb b/spec/unit/util/resolution_spec.rb
index 3fbf19e..36db3a3 100755
--- a/spec/unit/util/resolution_spec.rb
+++ b/spec/unit/util/resolution_spec.rb
@@ -284,6 +284,45 @@
Facter::Util::Resolution.should respond_to(:exec)
end
+ # taken from puppet: spec/unit/util_spec.rb
+ describe "#absolute_path?" do
+ describe "when run on unix" do
+ before :each do
+ Facter::Util::Config.stubs(:is_windows?).returns false
+ end
+
+ %w[/ /foo /foo/../bar //foo //Server/Foo/Bar //?/C:/foo/bar /\Server/Foo /foo//bar/baz].each do |path|
+ it "should return true for #{path}" do
+ Facter::Util::Resolution.should be_absolute_path(path)
+ end
+ end
+
+ %w[. ./foo \foo C:/foo \\Server\Foo\Bar \\?\C:\foo\bar \/?/foo\bar \/Server/foo foo//bar/baz].each do |path|
+ it "should return false for #{path}" do
+ Facter::Util::Resolution.should_not be_absolute_path(path)
+ end
+ end
+ end
+
+ describe "when run on windows" do
+ before :each do
+ Facter::Util::Config.stubs(:is_windows?).returns true
+ end
+
+ %w[C:/foo C:\foo \\\\Server\Foo\Bar \\\\?\C:\foo\bar //Server/Foo/Bar //?/C:/foo/bar /\?\C:/foo\bar \/Server\Foo/Bar c:/foo//bar//baz].each do |path|
+ it "should return true for #{path}" do
+ Facter::Util::Resolution.should be_absolute_path(path)
+ end
+ end
+
+ %w[/ . ./foo \foo /foo /foo/../bar //foo C:foo/bar foo//bar/baz].each do |path|
+ it "should return false for #{path}" do
+ Facter::Util::Resolution.should_not be_absolute_path(path)
+ end
+ end
+ end
+ end
+
# It's not possible, AFAICT, to mock %x{}, so I can't really test this bit.
describe "when executing code" do
it "should deprecate the interpreter parameter" do
@@ -294,5 +333,113 @@
it "should execute the binary" do
Facter::Util::Resolution.exec("echo foo").should == "foo"
end
+
+ describe "when run on unix" do
+ before :each do
+ Facter::Util::Config.stubs(:is_windows?).returns false
+ end
+
+ describe "and the binary is an absolute path" do
+ it "should run the command if the binary is found" do
+ File.expects(:executable?).with('/usr/bin/uname').returns true
+ Facter::Util::Resolution.expects(:`).with('/usr/bin/uname -a').returns "x86_64\n"
+ Facter::Util::Resolution.exec('/usr/bin/uname -a').should == 'x86_64'
+ end
+
+ # taken from the ip fact
+ it "should run more complicated shell _expression_" do
+ File.expects(:executable?).with('/sbin/arp').returns true
+ Facter::Util::Resolution.expects(:`).with('/sbin/arp -en -i eth0 | sed -e 1d').returns "some_data\n"
+ Facter::Util::Resolution.exec('/sbin/arp -en -i eth0 | sed -e 1d').should == 'some_data'
+ end
+
+ it "should not run the command if the binary is not present" do
+ File.expects(:executable?).with('/usr/bin/uname').returns false
+ Facter::Util::Resolution.expects(:`).with('/usr/bin/uname -a').never
+ Facter::Util::Resolution.exec('/usr/bin/uname -a').should be_nil
+ end
+ end
+
+ describe "and the binary is a relative path" do
+ it "should always include /sbin and /usr/sbin in search path" do
+ Facter::Util::Resolution.search_paths.should include '/sbin'
+ Facter::Util::Resolution.search_paths.should include '/usr/sbin'
+ end
+
+ it "should run the command if found in search path" do
+ Facter::Util::Resolution.stubs(:search_paths).returns ['/sbin', '/bin' ]
+ File.stubs(:executable?).with(File.join('/sbin','ifconfig')).returns false
+ File.stubs(:executable?).with(File.join('/bin','ifconfig')).returns true
+ Facter::Util::Resolution.expects(:`).with(File.join('/bin','ifconfig -a')).returns "done\n"
+ Facter::Util::Resolution.exec('ifconfig -a').should == 'done'
+ end
+
+ it "should not run the command if not found in any search path" do
+ Facter::Util::Resolution.stubs(:search_paths).returns ['/sbin', '/bin' ]
+ File.stubs(:executable?).with(File.join('/sbin','ifconfig')).returns false
+ File.stubs(:executable?).with(File.join('/bin','ifconfig')).returns false
+ Facter::Util::Resolution.exec('ifconfig -a').should be_nil
+ end
+ end
+ end
+
+ describe "when run on windows" do
+ before :each do
+ Facter::Util::Config.stubs(:is_windows?).returns true
+ end
+
+ describe "and the binary is an absolute path" do
+ it "should run the command if the binary is found" do
+ File.expects(:executable?).with('C:\foo\bar.exe').returns true
+ Facter::Util::Resolution.expects(:`).with('C:\foo\bar.exe /a /b /c "foo bar.txt"').returns "done\n"
+ Facter::Util::Resolution.exec('C:\foo\bar.exe /a /b /c "foo bar.txt"').should == 'done'
+ end
+
+ it "should handle quoted binaries with spaces correctly" do
+ File.expects(:executable?).with('C:\foo baz\bar.exe').returns true
+ Facter::Util::Resolution.expects(:`).with('"C:\foo baz\bar.exe" /a /b /c "foo bar.txt"').returns "done\n"
+ Facter::Util::Resolution.exec('"C:\foo baz\bar.exe" /a /b /c "foo bar.txt"').should == 'done'
+ end
+
+ it "should not run the command if the binary is not found" do
+ File.expects(:executable?).with('C:\foo\bar.exe').returns false
+ Facter::Util::Resolution.expects(:`).with('C:\foo\bar.exe /a /b /c "foo bar.txt"').never
+ Facter::Util::Resolution.exec('C:\foo\bar.exe /a /b /c "foo bar.txt"').should be_nil
+ end
+ end
+
+ describe "and the binary is a relative path" do
+ it "should run the command if found in search path" do
+ Facter::Util::Resolution.stubs(:search_paths).returns ['C:\Windows\system32', 'C:\Windows', 'C:\Windows\System32\Wbem' ]
+ File.stubs(:executable?).with(File.join('C:\Windows\system32','foo.exe')).returns false
+ File.stubs(:executable?).with(File.join('C:\Windows','foo.exe')).returns true
+ File.stubs(:executable?).with(File.join('C:\Windows\System32\Wbem', 'foo.exe')).returns false
+ Facter::Util::Resolution.expects(:`).with(File.join('C:\Windows','foo.exe')).returns "done\n"
+ Facter::Util::Resolution.exec('foo.exe').should == 'done'
+ end
+
+ it "should try to find the correct extension" do
+ ENV.stubs(:[]).with('PATHEXT').returns nil
+ Facter::Util::Resolution.stubs(:search_paths).returns ['C:\Windows\system32', 'C:\Windows']
+ ['.COM', '.EXE', '.BAT', '.CMD', '' ].each do |ext|
+ File.stubs(:executable?).with(File.join('C:\Windows\system32',"foo#{ext}")).returns false
+ end
+ ['.COM', '.BAT', '.CMD', '' ].each do |ext|
+ File.stubs(:executable?).with(File.join('C:\Windows',"foo#{ext}")).returns false
+ end
+ File.stubs(:executable?).with(File.join('C:\Windows',"foo.EXE")).returns true
+ Facter::Util::Resolution.expects(:`).with(File.join('C:\Windows','foo.EXE')).returns "done\n"
+ Facter::Util::Resolution.exec('foo').should == 'done'
+ end
+
+ it "should not run the command if not found in any search path" do
+ Facter::Util::Resolution.stubs(:search_paths).returns ['C:\Windows\system32', 'C:\Windows', 'C:\Windows\System32\Wbem' ]
+ File.stubs(:executable?).with(File.join('C:\Windows\system32','foo.exe')).returns false
+ File.stubs(:executable?).with(File.join('C:\Windows','foo.exe')).returns false
+ File.stubs(:executable?).with(File.join('C:\Windows\System32\Wbem', 'foo.exe')).returns false
+ Facter::Util::Resolution.exec('foo.exe').should be_nil
+ end
+ end
+ end
end
end
-- 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.
