Skip to content

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

Merged
silug merged 2 commits into
OpenVoxProject:mainfrom
adamboutcher:patch-1
Apr 25, 2025
Merged

Update util.rb to fix closing of file.#39
silug merged 2 commits into
OpenVoxProject:mainfrom
adamboutcher:patch-1

Conversation

@adamboutcher

Copy link
Copy Markdown
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
Copy Markdown
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

silug commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

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

adamboutcher commented Apr 22, 2025

Copy link
Copy Markdown
Contributor Author

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

silug commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

silug commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

@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

silug commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

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

adamboutcher commented Apr 22, 2025 via email

Copy link
Copy Markdown
Contributor Author

@adamboutcher

Copy link
Copy Markdown
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
Copy Markdown
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

ekohl commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

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

ekohl commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

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

Comment thread lib/puppet/util.rb
begin
Dir.foreach('/proc/self/fd') do |f|
if f != '.' && f != '..' && f.to_i >= 3
if %{^\d+$}.match?(f) && f.to_i >= 3

@ekohl ekohl Apr 29, 2025

Copy link
Copy Markdown
Contributor

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

ekohl commented May 1, 2025

Copy link
Copy Markdown
Contributor

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