Project

General

Profile

Misc #14907

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

Added by normalperson (Eric Wong) 4 months ago. Updated 3 months ago.

Status:
Closed
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

Associated revisions

Revision b53fadfd
Added by normal 3 months ago

process.c: defaults to close_others false

Arbitrarily closing file descriptors on exec breaks use cases
where a Ruby process sets up a descriptor for non-Ruby children
to use. For example, the "rake foo" target may spawn any number
of subprocesses (Ruby or not) which depends on parsing the "FOO"
environment variable for out_fd:99 and writing to foo.out

FOO=out_fd:99 rake foo 99>>foo.out

Unfortunately, this introduced one incompatibility in
test/lib/test/unit.rb and it now requires explicitly setting
IO#close_on_exec=true

[Misc #14907]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64399 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64399
Added by normalperson (Eric Wong) 3 months ago

process.c: defaults to close_others false

Arbitrarily closing file descriptors on exec breaks use cases
where a Ruby process sets up a descriptor for non-Ruby children
to use. For example, the "rake foo" target may spawn any number
of subprocesses (Ruby or not) which depends on parsing the "FOO"
environment variable for out_fd:99 and writing to foo.out

FOO=out_fd:99 rake foo 99>>foo.out

Unfortunately, this introduced one incompatibility in
test/lib/test/unit.rb and it now requires explicitly setting
IO#close_on_exec=true

[Misc #14907]

Revision 30ad3429
Added by normal 3 months ago

NEWS: clarify that we still use FD_CLOEXEC [ci skip]

[Misc #14907]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64408 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64408
Added by normalperson (Eric Wong) 3 months ago

NEWS: clarify that we still use FD_CLOEXEC [ci skip]

[Misc #14907]

History

#1 [ruby-core:87905] Updated by normalperson (Eric Wong) 4 months 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) 4 months ago

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

The second is w?

#3 [ruby-core:87910] Updated by normalperson (Eric Wong) 4 months 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) 4 months 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.

#6 [ruby-core:88367] Updated by akr (Akira Tanaka) 3 months ago

We discuss this issue at today's meeting.

We agree we can challenge to change the default.
Then, we can see actual problem (FD leak) exist or not.

#7 [ruby-core:88443] Updated by normalperson (Eric Wong) 3 months ago

akr@fsij.org wrote:

We agree we can challenge to change the default.
Then, we can see actual problem (FD leak) exist or not.

OK, I'll commit 0001-process.c-close_others-defaults-to-false.patch
early next week (won't have much Internet access soon)

#8 [ruby-core:88497] Updated by normalperson (Eric Wong) 3 months ago

I wrote:

OK, I'll commit 0001-process.c-close_others-defaults-to-false.patch

Nevermind, the version I uploaded here also introduces
compatibility problems around make jobserver FD inheritance with
rubyspec. So setting FD_CLOEXEC on IO.for_fd still introduces
incompatibilities.

So either I need to make changes to test/lib/test/unit.rb
or I make changes to spec/default.mspec
(for spec/ruby/optional/capi/spec_helper.rb)

Or we continue to suffer with the problem which caused me to
proposed this patch in the first place.

Anyways, this version which ONLY defaults "close_others: false"
and makes no change to IO.for_fd is likely the safest:

https://80x24.org/spew/20180815182638.8286-1-e@80x24.org/raw

But I still hate having to make the change to test/lib/test/unit.rb,
because this has a chance of breaking other 3rd-party code:

 diff --git a/test/lib/test/unit.rb b/test/lib/test/unit.rb
 index 51c8960c52..7a09466d1c 100644
 --- a/test/lib/test/unit.rb
 +++ b/test/lib/test/unit.rb
 @@ -145,6 +145,8 @@ def non_options(files, options)
 r.close if r
 nil
 else
 +            r.close_on_exec = true
 +            w.close_on_exec = true
 @jobserver = [r, w]
 options[:parallel] ||= 1
 end

Damned if we do, damned if we don't...

#9 [ruby-core:88501] Updated by Eregon (Benoit Daloze) 3 months ago

normalperson (Eric Wong) FWIW, I'm not fond of the jobserver-related logic in spec/ruby/optional/capi/spec_helper.rb, it's too complex and seems to gain nothing (but does prevent a make warning IIRC).
If there is an easy way to ignore the warning due to having a grandparent make process, I'll gladly take it. There is no point to parallelize building the C-API specs, it's single C files.

#10 [ruby-core:88508] Updated by normalperson (Eric Wong) 3 months ago

eregontp@gmail.com wrote:

normalperson (Eric Wong) FWIW, I'm not fond of the jobserver-related logic in spec/ruby/optional/capi/spec_helper.rb, it's too complex and seems to gain nothing (but does prevent a make warning IIRC).

It's fine, I guess; and it highlights the risk of this change.

The annoying thing about this problem is with my original goal
to fix a regression introduced in 2.0; we introduce a new
regression for code written after 2.0 (so I patch
test/lib/test/unit.rb ...)

#11 Updated by normalperson (Eric Wong) 3 months ago

  • Status changed from Open to Closed

Applied in changeset trunk|r64399.


process.c: defaults to close_others false

Arbitrarily closing file descriptors on exec breaks use cases
where a Ruby process sets up a descriptor for non-Ruby children
to use. For example, the "rake foo" target may spawn any number
of subprocesses (Ruby or not) which depends on parsing the "FOO"
environment variable for out_fd:99 and writing to foo.out

FOO=out_fd:99 rake foo 99>>foo.out

Unfortunately, this introduced one incompatibility in
test/lib/test/unit.rb and it now requires explicitly setting
IO#close_on_exec=true

[Misc #14907]

Also available in: Atom PDF