Avoid closing directory we're iterating

[ekohl]

This starts with reverting #39 because it's broken and caused a regression. Then I've taken my commits from puppetlabs/puppet#9546.

Ruby 3.4 started error checking directory access and starts to raise Errno::EBADF.

This particular loop iterates on all open file descriptors and one is the directory listing from Dir.foreach.

In the past this could have led to leaked file descriptors, but it's unlikely since it's likely the last opened file descriptor and have the highest number.

Link: ruby/ruby@f2919bd
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2349352

[ekohl]

And I just noticed this repo doesn't run any of the tests, but when I locally run it I did break them:

puppet/spec/unit/util_spec.rb

Lines 597 to 665 in 13dd914

	describe "safe_posix_fork", unless: Puppet::Util::Platform.windows? || Puppet::Util::Platform.jruby? do
	let(:pid) { 5501 }
	before :each do
	# Most of the things this method does are bad to do during specs. :/
	allow(Kernel).to receive(:fork).and_return(pid).and_yield
	allow($stdin).to receive(:reopen)
	allow($stdout).to receive(:reopen)
	allow($stderr).to receive(:reopen)
	# ensure that we don't really close anything!
	allow(IO).to receive(:new)
	end
	it "should close all open file descriptors except stdin/stdout/stderr when /proc/self/fd exists" do
	# This is ugly, but I can't really think of a better way to do it without
	# letting it actually close fds, which seems risky
	fds = [".", "..","0","1","2","3","5","100","1000"]
	fds.each do |fd|
	if fd == '.' || fd == '..'
	next
	elsif ['0', '1', '2'].include? fd
	expect(IO).not_to receive(:new).with(fd.to_i)
	else
	expect(IO).to receive(:new).with(fd.to_i).and_return(double('io', close: nil))
	end
	end
	dir_expectation = receive(:foreach).with('/proc/self/fd')
	fds.each do |fd|
	dir_expectation = dir_expectation.and_yield(fd)
	end
	allow(Dir).to dir_expectation
	Puppet::Util.safe_posix_fork
	end
	it "should close all open file descriptors except stdin/stdout/stderr when /proc/self/fd doesn't exist" do
	# This is ugly, but I can't really think of a better way to do it without
	# letting it actually close fds, which seems risky
	(0..2).each {|n| expect(IO).not_to receive(:new).with(n)}
	(3..256).each {|n| expect(IO).to receive(:new).with(n).and_return(double('io', close: nil)) }
	allow(Dir).to receive(:foreach).with('/proc/self/fd').and_raise(Errno::ENOENT)
	Puppet::Util.safe_posix_fork
	end
	it "should close all open file descriptors except stdin/stdout/stderr when /proc/self is not a directory" do
	# This is ugly, but I can't really think of a better way to do it without
	# letting it actually close fds, which seems risky
	(0..2).each {|n| expect(IO).not_to receive(:new).with(n)}
	(3..256).each {|n| expect(IO).to receive(:new).with(n).and_return(double('io', close: nil)) }
	allow(Dir).to receive(:foreach).with('/proc/self/fd').and_raise(Errno::ENOTDIR)
	Puppet::Util.safe_posix_fork
	end
	it "should fork a child process to execute the block" do
	expect(Kernel).to receive(:fork).and_return(pid).and_yield
	Puppet::Util.safe_posix_fork do
	"Fork this!"
	end
	end
	it "should return the pid of the child process" do
	expect(Puppet::Util.safe_posix_fork).to eq(pid)
	end
	end

[nmburgan]

@ekohl Have you been able to test this change and make sure it doesn't break anything? Looks okay to me, but I'm not super knowledgeable about this area.

[ekohl]

It's already rolled out in Fedora 42 and I wrote a test there. Without the patch anything that execs (like the exec resource) just straight up fails. With the patch it works again. See the linked bugzilla for more details. The micro-reproducers there give confidence it's correct

[jstraw]

https://docs.fedoraproject.org/en-US/defensive-coding/tasks/Tasks-Descriptors/#sect-Defensive_Coding-Tasks-Descriptors-Child_Processes talks about methodologies for this exact thing. It looks like ruby for some reason doesn't use FD_CLOEXEC/O_CLOEXEC to ensure they close on fork end.

[ekohl]

The Fedora page mentions that has a race condition. This implements the approach 2 it mentions: loop over /proc/self/fd with a fallback to static iteration.

My suggestion of using open3 would use a better system call to avoid all thr code, but is a much bigger refactor