(PUP-9997) Avoid Dir.chdir

lib/puppet/application/doc.rb

@@ -173,11 +173,7 @@ def other
 
  text += Puppet::Util::Reference.footer unless with_contents # We've only got one reference
 
- if options[:mode] == :pdf
- Puppet::Util::Reference.pdf(text)
- else
- puts text
- end
+ puts text
 
  exit exit_code
  end

lib/puppet/module_tool/tar/gnu.rb

@@ -4,18 +4,20 @@
 
 class Puppet::ModuleTool::Tar::Gnu
  def unpack(sourcefile, destdir, owner)
- sourcefile = File.expand_path(sourcefile)
+ safe_sourcefile = Shellwords.shellescape(File.expand_path(sourcefile))
  destdir = File.expand_path(destdir)
+ safe_destdir = Shellwords.shellescape(destdir)
 
- Dir.chdir(destdir) do
- Puppet::Util::Execution.execute("gzip -dc #{Shellwords.shellescape(sourcefile)} | tar xof -")
- Puppet::Util::Execution.execute("find . -type d -exec chmod 755 {} +")
- Puppet::Util::Execution.execute("find . -type f -exec chmod u+rw,g+r,a-st {} +")
- Puppet::Util::Execution.execute("chown -R #{owner} .")
- end
+ Puppet::Util::Execution.execute("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -")
+ Puppet::Util::Execution.execute(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+'])
+ Puppet::Util::Execution.execute(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+'])
+ Puppet::Util::Execution.execute(['chown', '-R', owner, destdir])
  end
 
  def pack(sourcedir, destfile)
- Puppet::Util::Execution.execute("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}")
+ safe_sourcedir = Shellwords.shellescape(sourcedir)
+ safe_destfile = Shellwords.shellescape(File.basename(destfile))
+
+ Puppet::Util::Execution.execute("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}")
  end
 end

lib/puppet/parser/functions/generate.rb

@@ -31,7 +31,8 @@
  end
 
  begin
- Dir.chdir(File.dirname(args[0])) { Puppet::Util::Execution.execute(args).to_str }
+ dir = File.dirname(args[0])
+ Puppet::Util::Execution.execute(args, failonfail: true, combine: true, cwd: dir).to_str
  rescue Puppet::ExecutionFailure => detail
  raise Puppet::ParseError, _("Failed to execute generator %{generator}: %{detail}") % { generator: args[0], detail: detail }, detail.backtrace
  end

lib/puppet/type/file/target.rb

@@ -44,22 +44,20 @@ def mklink
 
  raise Puppet::Error, "Could not remove existing file" if Puppet::FileSystem.exist?(@resource[:path])
 
- Dir.chdir(File.dirname(@resource[:path])) do
- Puppet::Util::SUIDManager.asuser(@resource.asuser) do
- mode = @resource.should(:mode)
- if mode
- Puppet::Util.withumask(0o00) do
- Puppet::FileSystem.symlink(target, @resource[:path])
- end
- else
+ Puppet::Util::SUIDManager.asuser(@resource.asuser) do
+ mode = @resource.should(:mode)
+ if mode
+ Puppet::Util.withumask(0o00) do
  Puppet::FileSystem.symlink(target, @resource[:path])
  end
+ else
+ Puppet::FileSystem.symlink(target, @resource[:path])
  end
+ end
 
-  @resource.send(:property_fix)
+ @resource.send(:property_fix)
 
- :link_created
- end
+ :link_created
  end
 
  def insync?(currentvalue)

lib/puppet/util/reference.rb

@@ -13,7 +13,7 @@ class Puppet::Util::Reference
  instance_load(:reference, 'puppet/reference')
 
  def self.modes
- %w[pdf text]
+ %w[text]
  end
 
  def self.newreference(name, options = {}, &block)
@@ -32,35 +32,6 @@ def self.page(*sections)
  end
  end
 
- def self.pdf(text)
- puts _("creating pdf")
- rst2latex = which('rst2latex') || which('rst2latex.py') ||
- raise(_("Could not find rst2latex"))
-
- cmd = %(#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex)
- Puppet::Util.replace_file("/tmp/puppetdoc.txt") { |f| f.puts text }
- # There used to be an attempt to use secure_open / replace_file to secure
- # the target, too, but that did nothing: the race was still here. We can
- # get exactly the same benefit from running this effort:
- begin
- Puppet::FileSystem.unlink('/tmp/puppetdoc.tex')
- rescue
- nil
- end
- output = %x(#{cmd})
- unless $CHILD_STATUS == 0
- $stderr.puts _("rst2latex failed")
- $stderr.puts output
- exit(1)
- end
- $stderr.puts output
-
- # Now convert to pdf
- Dir.chdir("/tmp") do
- %x(texi2pdf puppetdoc.tex >/dev/null 2>/dev/null)
- end
- end
-
  def self.references(environment)
  instance_loader(:reference).loadall(environment)
  loaded_instances(:reference).sort_by(&:to_s)

spec/unit/module_tool/tar/gnu_spec.rb

@@ -1,23 +1,27 @@
 require 'spec_helper'
 require 'puppet/module_tool'
 
-describe Puppet::ModuleTool::Tar::Gnu do
+describe Puppet::ModuleTool::Tar::Gnu, unless: Puppet::Util::Platform.windows? do
+ let(:sourcedir) { '/space path/the/src/dir' }
  let(:sourcefile) { '/space path/the/module.tar.gz' }
  let(:destdir) { '/space path/the/dest/dir' }
- let(:sourcedir) { '/space path/the/src/dir' }
- let(:destfile) { '/space path/the/dest/file.tar.gz' }
+ let(:destfile) { '/space path/the/dest/fi le.tar.gz' }
+
+ let(:safe_sourcedir) { '/space\ path/the/src/dir' }
+ let(:safe_sourcefile) { '/space\ path/the/module.tar.gz' }
+ let(:safe_destdir) { '/space\ path/the/dest/dir' }
+ let(:safe_destfile) { 'fi\ le.tar.gz' }
 
  it "unpacks a tar file" do
- expect(Dir).to receive(:chdir).with(File.expand_path(destdir)).and_yield
- expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{Shellwords.shellescape(File.expand_path(sourcefile))} | tar xof -")
- expect(Puppet::Util::Execution).to receive(:execute).with("find . -type d -exec chmod 755 {} +")
- expect(Puppet::Util::Execution).to receive(:execute).with("find . -type f -exec chmod u+rw,g+r,a-st {} +")
- expect(Puppet::Util::Execution).to receive(:execute).with("chown -R <owner:group> .")
+ expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -")
+ expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+'])
+ expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+'])
+ expect(Puppet::Util::Execution).to receive(:execute).with(['chown', '-R', '<owner:group>', destdir])
  subject.unpack(sourcefile, destdir, '<owner:group>')
  end
 
  it "packs a tar file" do
- expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}")
+ expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}")
  subject.pack(sourcedir, destfile)
  end
 end

spec/unit/parser/functions/generate_spec.rb

@@ -1,19 +1,41 @@
 require 'spec_helper'
 
+def with_executor
+ return yield unless Puppet::Util::Platform.jruby?
+
+ begin
+ Puppet::Util::ExecutionStub.set do |command, options, stdin, stdout, stderr|
+ require 'open3'
+ # simulate what puppetserver does
+ Dir.chdir(options[:cwd]) do
+ out, err, _status = Open3.capture3(*command)
+ stdout.write(out)
+ stderr.write(err)
+ # execution api expects stdout to be returned
+ out
+ end
+ end
+ yield
+ ensure
+ Puppet::Util::ExecutionStub.reset
+ end
+end
+
 describe "the generate function" do
  include PuppetSpec::Files
 
  let :node do Puppet::Node.new('localhost') end
  let :compiler do Puppet::Parser::Compiler.new(node) end
  let :scope do Puppet::Parser::Scope.new(compiler) end
+ let :cwd do tmpdir('generate') end
 
  it "should exist" do
  expect(Puppet::Parser::Functions.function("generate")).to eq("function_generate")
  end
 
  it "accept a fully-qualified path as a command" do
  command = File.expand_path('/command/foo')
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], anything).and_return("yay")
  expect(scope.function_generate([command])).to eq("yay")
  end
 
@@ -35,33 +57,66 @@
  end
 
  it "should execute the generate script with the correct working directory" do
- command = File.expand_path("/command")
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
- expect(scope.function_generate([command])).to eq('yay')
+ command = File.expand_path("/usr/local/command")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: %r{/usr/local})).and_return("yay")
+ scope.function_generate([command])
+ end
+
+ it "should execute the generate script with failonfail" do
+ command = File.expand_path("/usr/local/command")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(failonfail: true)).and_return("yay")
+ scope.function_generate([command])
+ end
+
+ it "should execute the generate script with combine" do
+ command = File.expand_path("/usr/local/command")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(combine: true)).and_return("yay")
+ scope.function_generate([command])
+ end
+
+ it "executes a command in a working directory" do
+ if Puppet::Util::Platform.windows?
+ command = File.join(cwd, 'echo.bat')
+ File.write(command, <<~END)
+ @echo off
+ echo %CD%
+ END
+ expect(scope.function_generate([command]).chomp).to match(cwd.gsub('/', '\\'))
+ else
+ with_executor do
+ command = File.join(cwd, 'echo.sh')
+ File.write(command, <<~END)
+ #!/bin/sh
+ echo $PWD
+ END
+ Puppet::FileSystem.chmod(0755, command)
+ expect(scope.function_generate([command]).chomp).to eq(cwd)
+ end
+ end
  end
 
  describe "on Windows", :if => Puppet::Util::Platform.windows? do
  it "should accept the tilde in the path" do
  command = "C:/DOCUME~1/ADMINI~1/foo.bat"
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "C:/DOCUME~1/ADMINI~1")).and_return("yay")
  expect(scope.function_generate([command])).to eq('yay')
  end
 
  it "should accept lower-case drive letters" do
  command = 'd:/command/foo'
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "d:/command")).and_return("yay")
  expect(scope.function_generate([command])).to eq('yay')
  end
 
  it "should accept upper-case drive letters" do
  command = 'D:/command/foo'
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "D:/command")).and_return("yay")
  expect(scope.function_generate([command])).to eq('yay')
  end
 
  it "should accept forward and backslashes in the path" do
  command = 'D:\command/foo\bar'
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: 'D:\command/foo')).and_return("yay")
  expect(scope.function_generate([command])).to eq('yay')
  end
 
@@ -81,7 +136,7 @@
 
  it "should accept plus and dash" do
  command = "/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-/foo"
- expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
+ expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: '/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-')).and_return("yay")
  expect(scope.function_generate([command])).to eq('yay')
  end
  end