Skip to content

Commit 36bdec0

Browse files
authored
Merge pull request #9387 from joshcooper/dir_chdir_9997
(PUP-9997) Avoid Dir.chdir
2 parents a43942b + 3c1cac6 commit 36bdec0

File tree

7 files changed

+100
-73
lines changed

7 files changed

+100
-73
lines changed

lib/puppet/application/doc.rb

+1-5
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,7 @@ def other
173173

174174
text += Puppet::Util::Reference.footer unless with_contents # We've only got one reference
175175

176-
if options[:mode] == :pdf
177-
Puppet::Util::Reference.pdf(text)
178-
else
179-
puts text
180-
end
176+
puts text
181177

182178
exit exit_code
183179
end

lib/puppet/module_tool/tar/gnu.rb

+10-8
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,20 @@
44

55
class Puppet::ModuleTool::Tar::Gnu
66
def unpack(sourcefile, destdir, owner)
7-
sourcefile = File.expand_path(sourcefile)
7+
safe_sourcefile = Shellwords.shellescape(File.expand_path(sourcefile))
88
destdir = File.expand_path(destdir)
9+
safe_destdir = Shellwords.shellescape(destdir)
910

10-
Dir.chdir(destdir) do
11-
Puppet::Util::Execution.execute("gzip -dc #{Shellwords.shellescape(sourcefile)} | tar xof -")
12-
Puppet::Util::Execution.execute("find . -type d -exec chmod 755 {} +")
13-
Puppet::Util::Execution.execute("find . -type f -exec chmod u+rw,g+r,a-st {} +")
14-
Puppet::Util::Execution.execute("chown -R #{owner} .")
15-
end
11+
Puppet::Util::Execution.execute("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -")
12+
Puppet::Util::Execution.execute(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+'])
13+
Puppet::Util::Execution.execute(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+'])
14+
Puppet::Util::Execution.execute(['chown', '-R', owner, destdir])
1615
end
1716

1817
def pack(sourcedir, destfile)
19-
Puppet::Util::Execution.execute("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}")
18+
safe_sourcedir = Shellwords.shellescape(sourcedir)
19+
safe_destfile = Shellwords.shellescape(File.basename(destfile))
20+
21+
Puppet::Util::Execution.execute("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}")
2022
end
2123
end

lib/puppet/parser/functions/generate.rb

+2-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@
3131
end
3232

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

lib/puppet/type/file/target.rb

+9-11
Original file line numberDiff line numberDiff line change
@@ -44,22 +44,20 @@ def mklink
4444

4545
raise Puppet::Error, "Could not remove existing file" if Puppet::FileSystem.exist?(@resource[:path])
4646

47-
Dir.chdir(File.dirname(@resource[:path])) do
48-
Puppet::Util::SUIDManager.asuser(@resource.asuser) do
49-
mode = @resource.should(:mode)
50-
if mode
51-
Puppet::Util.withumask(0o00) do
52-
Puppet::FileSystem.symlink(target, @resource[:path])
53-
end
54-
else
47+
Puppet::Util::SUIDManager.asuser(@resource.asuser) do
48+
mode = @resource.should(:mode)
49+
if mode
50+
Puppet::Util.withumask(0o00) do
5551
Puppet::FileSystem.symlink(target, @resource[:path])
5652
end
53+
else
54+
Puppet::FileSystem.symlink(target, @resource[:path])
5755
end
56+
end
5857

59-
@resource.send(:property_fix)
58+
@resource.send(:property_fix)
6059

61-
:link_created
62-
end
60+
:link_created
6361
end
6462

6563
def insync?(currentvalue)

lib/puppet/util/reference.rb

+1-30
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ class Puppet::Util::Reference
1313
instance_load(:reference, 'puppet/reference')
1414

1515
def self.modes
16-
%w[pdf text]
16+
%w[text]
1717
end
1818

1919
def self.newreference(name, options = {}, &block)
@@ -32,35 +32,6 @@ def self.page(*sections)
3232
end
3333
end
3434

35-
def self.pdf(text)
36-
puts _("creating pdf")
37-
rst2latex = which('rst2latex') || which('rst2latex.py') ||
38-
raise(_("Could not find rst2latex"))
39-
40-
cmd = %(#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex)
41-
Puppet::Util.replace_file("/tmp/puppetdoc.txt") { |f| f.puts text }
42-
# There used to be an attempt to use secure_open / replace_file to secure
43-
# the target, too, but that did nothing: the race was still here. We can
44-
# get exactly the same benefit from running this effort:
45-
begin
46-
Puppet::FileSystem.unlink('/tmp/puppetdoc.tex')
47-
rescue
48-
nil
49-
end
50-
output = %x(#{cmd})
51-
unless $CHILD_STATUS == 0
52-
$stderr.puts _("rst2latex failed")
53-
$stderr.puts output
54-
exit(1)
55-
end
56-
$stderr.puts output
57-
58-
# Now convert to pdf
59-
Dir.chdir("/tmp") do
60-
%x(texi2pdf puppetdoc.tex >/dev/null 2>/dev/null)
61-
end
62-
end
63-
6435
def self.references(environment)
6536
instance_loader(:reference).loadall(environment)
6637
loaded_instances(:reference).sort_by(&:to_s)

spec/unit/module_tool/tar/gnu_spec.rb

+13-9
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,27 @@
11
require 'spec_helper'
22
require 'puppet/module_tool'
33

4-
describe Puppet::ModuleTool::Tar::Gnu do
4+
describe Puppet::ModuleTool::Tar::Gnu, unless: Puppet::Util::Platform.windows? do
5+
let(:sourcedir) { '/space path/the/src/dir' }
56
let(:sourcefile) { '/space path/the/module.tar.gz' }
67
let(:destdir) { '/space path/the/dest/dir' }
7-
let(:sourcedir) { '/space path/the/src/dir' }
8-
let(:destfile) { '/space path/the/dest/file.tar.gz' }
8+
let(:destfile) { '/space path/the/dest/fi le.tar.gz' }
9+
10+
let(:safe_sourcedir) { '/space\ path/the/src/dir' }
11+
let(:safe_sourcefile) { '/space\ path/the/module.tar.gz' }
12+
let(:safe_destdir) { '/space\ path/the/dest/dir' }
13+
let(:safe_destfile) { 'fi\ le.tar.gz' }
914

1015
it "unpacks a tar file" do
11-
expect(Dir).to receive(:chdir).with(File.expand_path(destdir)).and_yield
12-
expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{Shellwords.shellescape(File.expand_path(sourcefile))} | tar xof -")
13-
expect(Puppet::Util::Execution).to receive(:execute).with("find . -type d -exec chmod 755 {} +")
14-
expect(Puppet::Util::Execution).to receive(:execute).with("find . -type f -exec chmod u+rw,g+r,a-st {} +")
15-
expect(Puppet::Util::Execution).to receive(:execute).with("chown -R <owner:group> .")
16+
expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -")
17+
expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+'])
18+
expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+'])
19+
expect(Puppet::Util::Execution).to receive(:execute).with(['chown', '-R', '<owner:group>', destdir])
1620
subject.unpack(sourcefile, destdir, '<owner:group>')
1721
end
1822

1923
it "packs a tar file" do
20-
expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}")
24+
expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}")
2125
subject.pack(sourcedir, destfile)
2226
end
2327
end

spec/unit/parser/functions/generate_spec.rb

+64-9
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,41 @@
11
require 'spec_helper'
22

3+
def with_executor
4+
return yield unless Puppet::Util::Platform.jruby?
5+
6+
begin
7+
Puppet::Util::ExecutionStub.set do |command, options, stdin, stdout, stderr|
8+
require 'open3'
9+
# simulate what puppetserver does
10+
Dir.chdir(options[:cwd]) do
11+
out, err, _status = Open3.capture3(*command)
12+
stdout.write(out)
13+
stderr.write(err)
14+
# execution api expects stdout to be returned
15+
out
16+
end
17+
end
18+
yield
19+
ensure
20+
Puppet::Util::ExecutionStub.reset
21+
end
22+
end
23+
324
describe "the generate function" do
425
include PuppetSpec::Files
526

627
let :node do Puppet::Node.new('localhost') end
728
let :compiler do Puppet::Parser::Compiler.new(node) end
829
let :scope do Puppet::Parser::Scope.new(compiler) end
30+
let :cwd do tmpdir('generate') end
931

1032
it "should exist" do
1133
expect(Puppet::Parser::Functions.function("generate")).to eq("function_generate")
1234
end
1335

1436
it "accept a fully-qualified path as a command" do
1537
command = File.expand_path('/command/foo')
16-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
38+
expect(Puppet::Util::Execution).to receive(:execute).with([command], anything).and_return("yay")
1739
expect(scope.function_generate([command])).to eq("yay")
1840
end
1941

@@ -35,33 +57,66 @@
3557
end
3658

3759
it "should execute the generate script with the correct working directory" do
38-
command = File.expand_path("/command")
39-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
40-
expect(scope.function_generate([command])).to eq('yay')
60+
command = File.expand_path("/usr/local/command")
61+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: %r{/usr/local})).and_return("yay")
62+
scope.function_generate([command])
63+
end
64+
65+
it "should execute the generate script with failonfail" do
66+
command = File.expand_path("/usr/local/command")
67+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(failonfail: true)).and_return("yay")
68+
scope.function_generate([command])
69+
end
70+
71+
it "should execute the generate script with combine" do
72+
command = File.expand_path("/usr/local/command")
73+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(combine: true)).and_return("yay")
74+
scope.function_generate([command])
75+
end
76+
77+
it "executes a command in a working directory" do
78+
if Puppet::Util::Platform.windows?
79+
command = File.join(cwd, 'echo.bat')
80+
File.write(command, <<~END)
81+
@echo off
82+
echo %CD%
83+
END
84+
expect(scope.function_generate([command]).chomp).to match(cwd.gsub('/', '\\'))
85+
else
86+
with_executor do
87+
command = File.join(cwd, 'echo.sh')
88+
File.write(command, <<~END)
89+
#!/bin/sh
90+
echo $PWD
91+
END
92+
Puppet::FileSystem.chmod(0755, command)
93+
expect(scope.function_generate([command]).chomp).to eq(cwd)
94+
end
95+
end
4196
end
4297

4398
describe "on Windows", :if => Puppet::Util::Platform.windows? do
4499
it "should accept the tilde in the path" do
45100
command = "C:/DOCUME~1/ADMINI~1/foo.bat"
46-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
101+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "C:/DOCUME~1/ADMINI~1")).and_return("yay")
47102
expect(scope.function_generate([command])).to eq('yay')
48103
end
49104

50105
it "should accept lower-case drive letters" do
51106
command = 'd:/command/foo'
52-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
107+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "d:/command")).and_return("yay")
53108
expect(scope.function_generate([command])).to eq('yay')
54109
end
55110

56111
it "should accept upper-case drive letters" do
57112
command = 'D:/command/foo'
58-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
113+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "D:/command")).and_return("yay")
59114
expect(scope.function_generate([command])).to eq('yay')
60115
end
61116

62117
it "should accept forward and backslashes in the path" do
63118
command = 'D:\command/foo\bar'
64-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
119+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: 'D:\command/foo')).and_return("yay")
65120
expect(scope.function_generate([command])).to eq('yay')
66121
end
67122

@@ -81,7 +136,7 @@
81136

82137
it "should accept plus and dash" do
83138
command = "/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-/foo"
84-
expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay")
139+
expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: '/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-')).and_return("yay")
85140
expect(scope.function_generate([command])).to eq('yay')
86141
end
87142
end

0 commit comments

Comments
 (0)