(PUP-9997) Avoid Dir.chdir

[joshcooper]

Dir.chdir is problematic because it affects all threads in the current process
and if puppet is started with a current working directory it doesn't have
traverse/execute permission to, then it won't be able to restore the cwd at the
end of the Dir.chdir block.

Puppet supports two execution implementations (posix and windows), both of which
already support the cwd option. Puppetserver also injects its external
implementation using the execution_stub. Its execution implementation now
supports the cwd option, see SERVER-3051.

Also pass the directory in which to extract modules to the tar command.

This should not be backported to 7.x

[ekohl]

I get you're not going to rename Puppet::ModuleTool::Tar::Gnu to Puppet::ModuleTool::Tar::Unix, but it would be nice if that was done eventually. Luckily the module tool isn't all that intensively used and I don't think the performance of executing a shell is a significant part of its overall performance.

On a related note: I have thought about a more generic gem/utility that tools like r10k can use to decouple from Puppet itself. For example, in our Puppet module acceptance tests I'd love to be able to trim the whole dependency tree down to remove Puppet.

But for now this looks like a good improvement. I just question if it's complete.

For example, there is:

puppet/lib/puppet/util/execution.rb

Lines 363 to 367 in 1a0ec94

	# We need to chdir to our cwd before changing privileges as there's a
	# chance that the user may not have permissions to access the cwd, which
	# would cause execute_posix to fail.
	cwd = options[:cwd]
	Dir.chdir(cwd) if cwd

This end up calling Kernel.exec but if I understand the documentation correctly that has the same signature as spawn, which takes a current_directory option. And an env option to remove the withenv option, but that's maybe a separate issue.

Then there's:

puppet/lib/puppet/util/reference.rb

Lines 58 to 61 in 1a0ec94

	# Now convert to pdf
	Dir.chdir("/tmp") do
	%x(texi2pdf puppetdoc.tex >/dev/null 2>/dev/null)
	end

This should probably use some execution helper instead of %x() (and respect TMPDIR).

The daemonize code is probably the most legitimate use of chdir and can stay:

puppet/lib/puppet/daemon.rb

Line 54 in 1a0ec94

Dir.chdir("/")

Last one is very questionable:

puppet/lib/puppet/type/file/target.rb

Line 47 in 1a0ec94

Dir.chdir(File.dirname(@resource[:path])) do

Because if you make target absolute then there's no need to chdir. Even more questionable: @resource.remove_existing(target) above it implies target is already absolute making the chdir redundant.

[joshcooper]

I just question if it's complete.

Thanks @ekohl, I appreciate your feedback. I'm thinking of next steps:

• The call to Kernel.exec happens inside a forked child. I'll look into passing the directory to the exec method
• The Puppet::Util::Reference code needs updating, I'll fix that
• Agree about daemonizing code
• The file target can be relative but the path should be absolute. Let me see if there's a way to resolve this

[joshcooper]

This end up calling Kernel.exec but if I understand the documentation correctly that has the same signature as spawn, which takes a current_directory option.

There's a comment that explains why we need to Dir.chdir before dropping privileges before calling Kernel.exec. The call also happens in a forked child, so won't affect the parent process.

This should probably use some execution helper instead of %x() (and respect TMPDIR).

This code was dead, so I deleted it

The daemonize code is probably the most legitimate use of chdir and can stay:

Yes, agreed. From https://man7.org/linux/man-pages/man7/daemon.7.html, "In the daemon process, change the current directory to the root directory (/), in order to avoid that the daemon involuntarily blocks mount points from being unmounted."

Because if you make target absolute then there's no need to chdir.

The Dir.chdir was introduced as a way to validate relative symlinks, but is no longer necessary. I deleted it in my PR. See commit message for details.