Tomoyuki Chikanaga <redmine / ruby-lang.org> 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 <normalperson / yhbt.net> 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