Skip to content

Update util.rb to fix closing of file. #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

adamboutcher
Copy link
Contributor

When attempting to close files, the /proc/self loop was trying to close a file being converted to an integer which was causing issues on Fedora 42, this resolved the problem.

Also see puppetlabs/puppet#9552

When attempting to close files, the /proc/self loop was trying to close a file being converted to an integer which was causing issues on Fedora 42, this resolved the problem.
@adamboutcher
Copy link
Contributor Author

For people searching this is the error that was flagging:

/usr/share/ruby/vendor_ruby/puppet/util.rb:481:in 'Dir.foreach': Bad file descriptor - closedir (Errno::EBADF)

@silug
Copy link
Contributor

silug commented Apr 22, 2025

If I understand this code correctly, it's trying to close each numbered file descriptor. The .to_i is likely appropriate, but the file list should be filtered to only include names that consist entirely of digits.

@adamboutcher
Copy link
Contributor Author

adamboutcher commented Apr 22, 2025

The function to close the file handle should be on the handle and not the handle converted to an integer, the to_i is being filtered in the if statement above?

So for example the code should be:
IO.new('/proc/self/fd/4').close
and not
IO.new(4).close

However I'm not a ruby dev so maybe my understanding of the code is incorrect

@silug
Copy link
Contributor

silug commented Apr 22, 2025

I think what it is doing is closing all file descriptors other than stdin, stdout, stderr, and whatever fd 3 is. 🙂

It does not use the full path in /proc/self/fd, just the numeric file descriptors. On Linux, scanning /proc/self/fd is a handy way to do that.

@adamboutcher
Copy link
Contributor Author

I think what it is doing is closing all file descriptors other than stdin, stdout, stderr, and whatever fd 3 is. 🙂

I think that's what is intended but the to_i is possibly getting in the way...

I have Fedora 40 which does work (or silently fails) where as Fedora 42 gives the aforementioned error.
The modification in the PR works on both systems fine or is silently failing.

@silug
Copy link
Contributor

silug commented Apr 22, 2025

@adamboutcher Could you try adding ${^\d+$}.match?(f) to the condition on line 482? I suspect that will resolve the underlying problem.

The line should probably look like this:

          if %{^\d+$}.match?(f) && f.to_i >= 3

@silug
Copy link
Contributor

silug commented Apr 22, 2025

To be clear, I think the change should look like this:

diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 06c7b11815..66be0f7da3 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -479,7 +479,7 @@ module Util
 
       begin
         Dir.foreach('/proc/self/fd') do |f|
-          if f != '.' && f != '..' && f.to_i >= 3
+          if %{^\d+$}.match?(f) && f.to_i >= 3
             begin
               IO.new(f.to_i).close
             rescue

But I'm not seeing this problem for whatever reason, so I can't easily test it.

@adamboutcher
Copy link
Contributor Author

adamboutcher commented Apr 22, 2025 via email

@adamboutcher
Copy link
Contributor Author

if %{^\d+$}.match?(f) && f.to_i >= 3

I've tried this on Fedora42 and it seems to work, or at least I dont get any errors...
I'll change up and repush.

adamboutcher added a commit to adamboutcher/puppet that referenced this pull request Apr 25, 2025
@nmburgan
Copy link
Member

@ekohl Do you see any benefit to adding some of your logic from your other PR here? I think they both accomplish the goal. This one guards against any non-integer FD, but yours specifically filters out the ones we expect. I'm not sure if there are other edge cases we need to consider.

@ekohl
Copy link
Contributor

ekohl commented Apr 29, 2025

I don't think this actually resolves it. The file descriptor is the last number. If this avoids the error I think it won't close anything at all anymore. I'd suggest reverting this and applying my patch. See the linked bugzilla for minimal reproducer code

@ekohl
Copy link
Contributor

ekohl commented Apr 29, 2025

Based on https://bugzilla.redhat.com/show_bug.cgi?id=2349352#c9 a reproducer to show this broke the code:

#!/usr/bin/env ruby

d = Dir.new('/proc/self/fd')
d.each_child do |f|
  if %{^\d+$}.match?(f) && f.to_i >= 3
    begin
      puts "Closing #{f}"
      IO.new(f.to_i).close
    rescue
      nil
    end
  else
    puts "Not closing #{f}"
  end
end

Running it shows it doesn't close anything anymore:

$ ruby close1.rb
Not closing 0
Not closing 1
Not closing 2
Not closing 3
Not closing 4
Not closing 5

@@ -479,7 +479,7 @@ def safe_posix_fork(stdin = $stdin, stdout = $stdout, stderr = $stderr, &block)

begin
Dir.foreach('/proc/self/fd') do |f|
if f != '.' && f != '..' && f.to_i >= 3
if %{^\d+$}.match?(f) && f.to_i >= 3
Copy link
Contributor

@ekohl ekohl Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want this to work, it needs to be

Suggested change
if %{^\d+$}.match?(f) && f.to_i >= 3
if %r{^\d+$}.match?(f) && f.to_i >= 3

Edit: but then you run into the original issue again, which is that the last file descriptor can't be closed.

@ekohl
Copy link
Contributor

ekohl commented May 1, 2025

I've opened #42 to properly fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants