Project

General

Profile

Misc #14907

[PATCH] io.c: do not close inherited FDs by default

Added by normalperson (Eric Wong) 9 days ago. Updated 1 day ago.

Status:
Open
Priority:
Normal
[ruby-core:87904]

Description

io.c: do not close inherited FDs by default

While I fully agree Ruby should create FDs with close-on-exec by
default (as it has since 2.0.0); I don't believe Ruby should be
arbitrarily closing file descriptors it does not know about.

The following is an example (using mwrap[1]) where closing
inherited FDs is harmful.

MWRAP=dump_fd:99 mwrap make -j4 exam 99>>mwrap.out

I only found one minor regression from this change in
test/lib/test/unit.rb as IO.new does not set close-on-exec
when using jobserver FDs from make. A possible change
is to make IO.new set FD_CLOEXEC by default.

[1] https://80x24.org/mwrap/README.html

History

#1 [ruby-core:87905] Updated by normalperson (Eric Wong) 9 days ago

https://bugs.ruby-lang.org/issues/14907

I only found one minor regression from this change in
test/lib/test/unit.rb as IO.new does not set close-on-exec
when using jobserver FDs from make. A possible change
is to make IO.new set FD_CLOEXEC by default.

Maybe this is a good complimentary change:

diff --git a/ext/socket/basicsocket.c b/ext/socket/basicsocket.c
index 2641b4410b..0418c022cf 100644
--- a/ext/socket/basicsocket.c
+++ b/ext/socket/basicsocket.c
@@ -28,6 +28,7 @@ bsock_s_for_fd(VALUE klass, VALUE fd)
     VALUE sock = rsock_init_sock(rb_obj_alloc(klass), NUM2INT(fd));

     GetOpenFile(sock, fptr);
+    rb_maygvl_fd_fix_cloexec(fptr->fd);

     return sock;
 }
diff --git a/io.c b/io.c
index b041acdef0..ddebd09e65 100644
--- a/io.c
+++ b/io.c
@@ -8085,6 +8085,7 @@ rb_io_initialize(int argc, VALUE *argv, VALUE io)
 #else
     if (fstat(fd, &st) == -1) rb_sys_fail(0);
 #endif
+    rb_maygvl_fd_fix_cloexec(fd);
     rb_update_max_fd(fd);
 #if defined(HAVE_FCNTL) && defined(F_GETFL)
     ofmode = rb_io_oflags_fmode(oflags);
diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb
index a6a8e78209..97ea857c3e 100644
--- a/test/ruby/test_io.rb
+++ b/test/ruby/test_io.rb
@@ -3128,6 +3128,15 @@ def test_cloexec
     }
   end

+  def test_for_fd_close_on_exec
+    IO.pipe do |r, w|
+      w.close_on_exec = false
+      w2 = IO.new(w.fileno, autoclose: false)
+      assert_predicate w, :close_on_exec?
+      assert_predicate w2, :close_on_exec?
+    end
+  end if have_close_on_exec?
+
   def test_ioctl_linux
     # Alpha, mips, sparc and ppc have an another ioctl request number scheme.
     # So, hardcoded 0x80045200 may fail.
diff --git a/test/socket/test_basicsocket.rb b/test/socket/test_basicsocket.rb
index d388b4f0dd..6ea8bcfdf3 100644
--- a/test/socket/test_basicsocket.rb
+++ b/test/socket/test_basicsocket.rb
@@ -150,6 +150,13 @@ def test_for_fd
       s = BasicSocket.for_fd(sock.fileno)
       assert_instance_of BasicSocket, s
       s.autoclose = false
+
+      sock.close_on_exec = false
+      s2 = BasicSocket.for_fd(sock.fileno)
+      s2.autoclose = false
+      assert_predicate s2, :close_on_exec?
+      assert_predicate sock, :close_on_exec?
+
       sock.close
     end
   end

#2 [ruby-core:87909] Updated by nobu (Nobuyoshi Nakada) 9 days ago

+            r.close_on_exec = true
+            r.close_on_exec = true

The second is w?

#3 [ruby-core:87910] Updated by normalperson (Eric Wong) 9 days ago

nobu@ruby-lang.org wrote:

+            r.close_on_exec = true
+            r.close_on_exec = true

The second is w?

Yes :) But I prefer we drop this hunk and use make IO#new
set FD_CLOEXEC

#4 [ruby-core:88007] Updated by akr (Akira Tanaka) 1 day ago

I understand that FD inheritance dependent program doesn't work well with Ruby.

But I doubt that all external library create FDs with close-on-exec now.
spawn's close_others option would be useful workaround to avoid FD leak
with such (problematic) library.

So, I'm reluctant that removing close_others feature.
It means that we lose the workaround.

However, turn off close_others by default (for spawn and IO.popen) seems possible choice for me.

It is difficult to see how many the problematic libraries actually exists
with closing FDs by default.

Also available in: Atom PDF