Feature #2250: IO::for_fd() objects' finalization dangerously closes underlying fds

• Status: Rejected
• Assignee: Eregon (Benoit Daloze)
• Target version: -

[pilcrow (Mike Pomraning)]

=begin

1. Expected behavior:

An IO object created by IO::for_fd(a_socket_fd) should not call close(2) on its underlying fd upon finalization. The object did not allocate the fd, and so cannot safely nor politely close it.

2. Observed behavior:

Instead, an IO object created by IO::for_fd(a_socket_fd) will attempt to close(2) its underlying socket fd upon finalization.

3. How to reproduce:

The attached script and trivial extension module demonstrate that an IO::for_fd()-created object may secretly close(2) the fd behind an innocent File.new(...) object, causing operations on that File object to fail for no reason apparent in (nor knowable to) the ruby code. On 32-bit Linux under ruby-1.9 (or 1.8. for that matter), it produces:

$ ruby io-finalize.rb
#<File::Stat dev=0x11, ino=1193, ....
Finalizing an IO::for_fd() object...
io-finalize.rb:29:in `stat': Bad file descriptor - /dev/null (Errno::EBADF)
from io-finalize.rb:29

4. Why this is very bad:

In practice, fds passed to for_fd() may be exposed by extension modules, which modules are responsible for the cleanup of the file descriptor. Thus, close(2)ing upon finalization may rudely and dangerously close a file descriptor already closed and reassigned to some unrelated bit of code, causing baffling, "action at a distance" failures.

Indeed, the script to reproduce this failure simulates the experience of two different users of two different ruby bindings to PostgreSQL's libpq (ruby-pg and dbd-altpg), whose uses of Kernel.select( [ IO.for_fd(pg_underlying_socket) ] ... ) thus caused failures elsewhere.

Casual testing suggests that this behavior also applies to at least one other non-regular file type, the FIFO.
=end

[normalperson (Eric Wong)]

=begin
Mike Pomraning [e-mail] wrote:

Bug #2250: IO::for_fd() objects' finalization dangerously closes underlying fds
http://redmine.ruby-lang.org/issues/show/2250

2. Observed behavior:

Instead, an IO object created by IO::for_fd(a_socket_fd) will attempt
to close(2) its underlying socket fd upon finalization.

FWIW, I've been sticking IO objects created with IO.for_fd into a
global Array to workaround this behavior.

4. Why this is very bad:

I don't have an opinion on whether this behavior is bad or not. I would
avoid repeatedly calling IO.for_fd on the same underlying file
descriptor regardless because it's less memory thrashing that way.

=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Thu, 22 Oct 2009 12:43:37 +0900,
Mike Pomraning wrote in [ruby-core:26222]:

4. Why this is very bad:

In practice, fds passed to for_fd() may be exposed by
extension modules, which modules are responsible for the
cleanup of the file descriptor. Thus, close(2)ing upon
finalization may rudely and dangerously close a file
descriptor already closed and reassigned to some unrelated
bit of code, causing baffling, "action at a distance"
failures.

IO.for_fd is often dangerous. Basically, such extension
libraries should expose IO instances, but not file descriptors.

This is a patch to add :autoclose option to IO.for_fd and
IO.new.

Index: io.c

--- io.c (revision 25430)
+++ io.c (working copy)
@@ -129,5 +129,5 @@ static VALUE argf;
static ID id_write, id_read, id_getc, id_flush, id_readpartial;
static VALUE sym_mode, sym_perm, sym_extenc, sym_intenc, sym_encoding, sym_open_args;
-static VALUE sym_textmode, sym_binmode;
+static VALUE sym_textmode, sym_binmode, sym_autoclose;

struct timeval rb_time_interval(VALUE);
@@ -4296,4 +4296,7 @@ extract_binmode(VALUE opthash, int *fmod
if ((*fmode & FMODE_BINMODE) && (*fmode & FMODE_TEXTMODE))
rb_raise(rb_eArgError, "both textmode and binmode specified");

• v = rb_hash_aref(opthash, sym_autoclose);
• if (v == Qfalse)

•  *fmode |= FMODE_PREP;
  

  }
  }
  @@ -9936,3 +9939,4 @@ Init_IO(void)
  sym_textmode = ID2SYM(rb_intern("textmode"));
  sym_binmode = ID2SYM(rb_intern("binmode"));
• sym_autoclose = ID2SYM(rb_intern("autoclose"));
  }
  

--
Nobu Nakada

=end

[pilcrow (Mike Pomraning)]

=begin
In comment 2 Nobuyoshi Nakada writes:

This is a patch to add :autoclose option to IO.for_fd

Thanks! IIUC, your patch requires one to explicitly set
:autoclose => false to avoid this behavior. However, I'd
argue that no autoclose ought to be the default behavior.

IMHO, if you know enough about fds to dress one up as an
IO, you know enough to judge whether Ruby should close it
for you. Ruby can never know this a priori, and in most
cases I can think of, it isn't ever desired.

Indeed, a 2002 commit message suggests that not closing
should be the default:

| Revision 2312 matz, 04/01/2002 01:39 AM
|
| * re.c (match_setter): it's OK to assign nil to $~.
| * io.c (rb_io_fptr_cleanup): do not close IO created by for_fd().
| * io.c (rb_io_initialize): mark IO created by for_fd
| * ext/socket/socket.c (bsock_s_for_fd): ditto.

Basically, such extension libraries should expose IO
instances, but not file descriptors.

Where practicable, yes. Respectfully and UIMS, however,
I think this asks too much of ext library authors.

Either they must wait for and explicitly use your
autoclose feature, or must manage their own IO object
references -- and the binding authors themselves may
only have a "naked" fd to play with, may not know when
the underlying library chooses to call close(), etc.

Regards,
Mike
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Fri, 23 Oct 2009 07:05:23 +0900,
Mike Pomraning wrote in [ruby-core:26242]:

IMHO, if you know enough about fds to dress one up as an
IO, you know enough to judge whether Ruby should close it
for you. Ruby can never know this a priori, and in most
cases I can think of, it isn't ever desired.

I don't think it is acceptable, because of backward
compatibility.
Excerpt from rdoc of Socket#sysaccept:

   client_fd, client_sockaddr = socket.sysaccept
   client_socket = Socket.for_fd( client_fd )

This example expects the fd will get closed automatically.

Indeed, a 2002 commit message suggests that not closing
should be the default:

| Revision 2312 matz, 04/01/2002 01:39 AM
|
| * re.c (match_setter): it's OK to assign nil to $~.
| * io.c (rb_io_fptr_cleanup): do not close IO created by for_fd().
| * io.c (rb_io_initialize): mark IO created by for_fd
| * ext/socket/socket.c (bsock_s_for_fd): ditto.

It's been removed soon.

r2375 | matz | 2002-04-15 16:48:47 +0900 (Mon, 15 Apr 2002) | 9 lines

• io.c (rb_io_fptr_cleanup): should close IO created by IO.new(fd).

• rubyio.h: remove FMODE_FDOPEN

---

Basically, such extension libraries should expose IO
instances, but not file descriptors.

Where practicable, yes. Respectfully and UIMS, however,
I think this asks too much of ext library authors.

Hmm, there is no one-liner function, indeed.

#ifdef GetWriteFile
static void
fptr_finalize_noclose(rb_io_t *fptr, int noraise)
{
fflush(GetWriteFile(fptr));
}
#endif

VALUE
make_noclose_io(int fd, int mode)
{
VALUE io;
rb_io_t *fptr;
MakeOpenFile(io, fptr);
fptr->fd = fd;
#ifdef GetWriteFile
fptr->finalize = fptr_finalize_noclose;
#else
mode |= FMODE_PREP;
#endif
fptr->mode |= mode;
return io;
}

Either they must wait for and explicitly use your
autoclose feature, or must manage their own IO object
references -- and the binding authors themselves may
only have a "naked" fd to play with, may not know when
the underlying library chooses to call close(), etc.

If the fd is bound to an IO instance or not, the library can
close it always.

--
Nobu Nakada

=end

[hongli (Hongli Lai)]

=begin
Indeed. I have some code which expect the fd to be auto-closed. Autoclose should be on by default.
=end

[mame (Yusuke Endoh)]

• Assignee set to nobu (Nobuyoshi Nakada)
• Priority changed from 5 to Normal

=begin
Hi nobu,

This is a patch to add :autoclose option to IO.for_fd and
IO.new.

There seems to be no objection. Why don't you commit the patch?

Hmm, there is no one-liner function, indeed.

I agree with providing the API.

--
Yusuke Endoh [e-mail]
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Thu, 18 Mar 2010 23:21:00 +0900,
Yusuke Endoh wrote in [ruby-core:28750]:

This is a patch to add :autoclose option to IO.for_fd and
IO.new.

There seems to be no objection. Why don't you commit the patch?

Mike didn't seem to agree it. If he compromises with it, I'll
commit it soon.

My opinion is:

• agreed that the way not to close automatically would be
  necessary in some case
• but it can't be default because of backward compatibility,
• and, bare fd should not appear in ruby level elementarily
  therefore it's an issue of such extension library.

--
Nobu Nakada

=end

[mame (Yusuke Endoh)]

=begin
Hi,

2010/3/19 Nobuyoshi Nakada [e-mail]:

Mike didn't seem to agree it. If he compromises with it, I'll
commit it soon.

Regardless of whether he agrees or not :-), the feature or substitute is
actually needed in the case he mentioned, isn't it?

I thought this issue is urgent because it is difficult for users to avoid
this issue by some workaround. If it is not, I'll change the target to
1.9.x.

My opinion is:

• agreed that the way not to close automatically would be
  necessary in some case
• but it can't be default because of backward compatibility,
• and, bare fd should not appear in ruby level elementarily
  therefore it's an issue of such extension library.

Completely agreed.

--
Yusuke ENDOH [e-mail]

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
This issue was solved with changeset r26999.
Mike, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

[Eregon (Benoit Daloze)]

• Description updated (diff)

Here is my opinion, from the experience of chasing many IO.for_fd bugs due to this behavior.
I agree with the reporter, it doesn't make sense to autoclose a fd which is not owned (for_fd didn't create it, something else did, and it's extremely likely something else should/will close it).

In tests/specs, this is easily the worse kind of bug to track down, because the autoclose on GC will close another fd, randomly, and it's very hard to track down, even with LeakChecker.
Essential, I believe every use of for_fd without autoclose=false is a bug.
And I would argue the same for applications.

@hongli (Hongli Lai) Do you have an actual example where the current behavior is useful?

I think it's OK in the very rare cases where we want to have ownership of a fd we did not open to do io.autoclose = true.

I think the Socket#sysaccept documentation example is silly and not good practice.
It should just do client_socket.close, which is what the doc of TCPServer#sysaccept already does.
Closing reliably is anyway better for many reasons, such as avoiding fd exhaustion, quick release of resources, etc.

So, I want to challenge the default here to be sane behavior.
Applications which do not care about fast release of resources (as they rely on finalization close) and want to get ownership of a FD should set io.autolose = true.
I expect they are extremely rare.
And it's easier to debug a FD leak than randomly-closed unrelated fds at a later point in time by the GC.

[Eregon (Benoit Daloze)]

• Status changed from Closed to Open

Let me reopen this since the default is still dangerous and the original report is not solved.

[Eregon (Benoit Daloze)]

• Assignee changed from nobu (Nobuyoshi Nakada) to Eregon (Benoit Daloze)

[akr (Akira Tanaka)]

• Status changed from Open to Rejected

We discussed this issue today's Ruby developer meeting.

Our conclusion is that changing the default behavior of IO.for_fd is
too incompatible. It changes good working program to FD-leaking program.
It is another big issue, especially problematic for long-running server
and very very difficult to debug.