Merge pull request #9387 from joshcooper/dir_chdir_9997

(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