[ruby-core:35669] Re: [Ruby 1.9 - Bug #4558] TestSocket#test_closed_read fails after r31230

Tomoyuki Chikanaga <[e-mail]> wrote:
> ruby -v changed from - to ruby 1.9.3dev (2011-04-05 trunk 31241) [x86_64-darwin10.6.0]
> 
> Hi,
> 
> I applied Eric's patch, but TestSocket#test_closed_read still report same failure.
> 
> I can reproduce EBADF with following script.

Thanks for testing, think I have a better fix below (supercedes my
original fix)

Also pushed to the "io-close-fixes" branch of git://bogomips.org/ruby.git

From d5f9659ea9c2e8e0ed67544ed35afef4ca2bb3c5 Mon Sep 17 00:00:00 2001
From: Eric Wong <[e-mail]>
Date: Thu, 7 Apr 2011 19:25:20 +0000
Subject: [PATCH] io.c (rb_io_close): ensure IOError for cross-thread closes

We need to inform threads to stop operations on the FD before
closing it and also invalidate the fd member of the rb_io_t
struct for other threads to properly raise IOError.

FDs may be created and destroyed without the GVL, so
rb_thread_fd_close() may be improperly hitting the wrong
threads/FDs if we close() before notifying and in the worst case
or threads will end up reading/writing to an unexpected FD.

ref: [ruby-core:35631]
ref: http://redmine.ruby-lang.org/issues/4558
---
 io.c                 |   25 ++++++++++++++++++-------
 test/ruby/test_io.rb |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/io.c b/io.c
index 7ce7148..5d37b7f 100644
--- a/io.c
+++ b/io.c
@@ -3504,6 +3504,7 @@ maygvl_close(int fd, int keepgvl)
     if (keepgvl)
 	return close(fd);
 
+    rb_thread_fd_close(fd);
     /*
      * close() may block for certain file types (NFS, SO_LINGER sockets,
      * inotify), so let other threads run.
@@ -3525,6 +3526,8 @@ maygvl_fclose(FILE *file, int keepgvl)
     if (keepgvl)
 	return fclose(file);
 
+    rb_thread_fd_close(fileno(file));
+
     return (int)rb_thread_blocking_region(nogvl_fclose, file, RUBY_UBF_IO, 0);
 }
 
@@ -3555,24 +3558,35 @@ fptr_finalize(rb_io_t *fptr, int noraise)
 	}
     }
     if (IS_PREP_STDIO(fptr) || fptr->fd <= 2) {
+        int fd = fptr->fd;
+
+        fptr->stdio_file = 0;
+        fptr->fd = -1;
+        rb_thread_fd_close(fd);
         goto skip_fd_close;
     }
     if (fptr->stdio_file) {
+	FILE *stdio_file = fptr->stdio_file;
+
+	fptr->stdio_file = 0;
+	fptr->fd = -1;
+
         /* fptr->stdio_file is deallocated anyway
          * even if fclose failed.  */
-	if ((maygvl_fclose(fptr->stdio_file, noraise) < 0) && NIL_P(err))
+	if ((maygvl_fclose(stdio_file, noraise) < 0) && NIL_P(err))
             err = noraise ? Qtrue : INT2NUM(errno);
     }
     else if (0 <= fptr->fd) {
+	int fd = fptr->fd;
+	fptr->fd = -1;
+
         /* fptr->fd may be closed even if close fails.
          * POSIX doesn't specify it.
          * We assumes it is closed.  */
-	if ((maygvl_close(fptr->fd, noraise) < 0) && NIL_P(err))
+	if ((maygvl_close(fd, noraise) < 0) && NIL_P(err))
 	    err = noraise ? Qtrue : INT2NUM(errno);
     }
   skip_fd_close:
-    fptr->fd = -1;
-    fptr->stdio_file = 0;
     fptr->mode &= ~(FMODE_READABLE|FMODE_WRITABLE);
 
     if (!NIL_P(err) && !noraise) {
@@ -3668,7 +3682,6 @@ VALUE
 rb_io_close(VALUE io)
 {
     rb_io_t *fptr;
-    int fd;
     VALUE write_io;
     rb_io_t *write_fptr;
 
@@ -3684,9 +3697,7 @@ rb_io_close(VALUE io)
     if (!fptr) return Qnil;
     if (fptr->fd < 0) return Qnil;
 
-    fd = fptr->fd;
     rb_io_fptr_cleanup(fptr, FALSE);
-    rb_thread_fd_close(fd);
 
     if (fptr->pid) {
 	rb_syswait(fptr->pid);
diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb
index 6b8e6b5..3d086b3 100644
--- a/test/ruby/test_io.rb
+++ b/test/ruby/test_io.rb
@@ -1809,4 +1809,43 @@ End
       Process.waitpid2(pid)
     end
   end
+
+  def test_cross_thread_close_fd
+    with_pipe do |r,w|
+      read_thread = Thread.new do
+        begin
+          r.read(1)
+        rescue => e
+          e
+        end
+      end
+
+      sleep(0.1) until read_thread.stop?
+      r.close
+      read_thread.join
+      assert_kind_of(IOError, read_thread.value)
+    end
+  end
+
+  def test_cross_thread_close_stdio
+    with_pipe do |r,w|
+      pid = fork do
+        $stdin.reopen(r)
+        r.close
+        read_thread = Thread.new do
+          begin
+            $stdin.read(1)
+          rescue => e
+            e
+          end
+        end
+        sleep(0.1) until read_thread.stop?
+        $stdin.close
+        read_thread.join
+        exit(IOError === read_thread.value)
+      end
+      assert Process.waitpid2(pid)[1].success?
+    end
+    rescue NotImplementedError
+  end
 end
-- 
Eric Wong