diff --git a/ChangeLog b/ChangeLog index 16d43145dd..2fb6ec3b65 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Mon Oct 10 15:22:27 2016 Nobuyoshi Nakada + + * ruby.c (open_load_file): bind the open fd to an IO instance + before waiting FIFO, not to leak the fd if interrupted. + Mon Oct 10 12:40:54 2016 Nobuyoshi Nakada * ruby.c (open_load_file): compare with EXEEXT instead of hard diff --git a/ruby.c b/ruby.c index 8678bcce2c..287e22aaaa 100644 --- a/ruby.c +++ b/ruby.c @@ -1916,22 +1916,20 @@ open_load_file(VALUE fname_v, int *xflag) #endif e = ruby_is_fd_loadable(fd); - if (e <= 0) { - if (!e) { - e = errno; - (void)close(fd); - rb_load_fail(fname_v, strerror(e)); - } - else { - /* - We need to wait if FIFO is empty. It's FIFO's semantics. - rb_thread_wait_fd() release GVL. So, it's safe. - */ - rb_thread_wait_fd(fd); - } + if (!e) { + e = errno; + (void)close(fd); + rb_load_fail(fname_v, strerror(e)); } f = rb_io_fdopen(fd, mode, fname); + if (e < 0) { + /* + We need to wait if FIFO is empty. It's FIFO's semantics. + rb_thread_wait_fd() release GVL. So, it's safe. + */ + rb_thread_wait_fd(fd); + } } return f; } diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb index b5802e71c4..5c2221c77b 100644 --- a/test/ruby/test_require.rb +++ b/test/ruby/test_require.rb @@ -757,6 +757,26 @@ class TestRequire < Test::Unit::TestCase } end if File.respond_to?(:mkfifo) + def test_loading_fifo_fd_leak + Tempfile.create(%w'fifo .rb') {|f| + f.close + File.unlink(f.path) + File.mkfifo(f.path) + assert_separately(["-", f.path], "#{<<-"begin;"}\n#{<<-"end;"}", timeout: 3) + begin; + Process.setrlimit(Process::RLIMIT_NOFILE, 50) + th = Thread.current + 100.times do |i| + Thread.start {begin sleep(0.001) end until th.stop?; th.raise(IOError)} + assert_raise(IOError, "\#{i} time") do + tap {tap {tap {load(ARGV[0])}}} + end + GC.start + end + end; + } + end if File.respond_to?(:mkfifo) and defined?(Process::RLIMIT_NOFILE) + def test_throw_while_loading Tempfile.create(%w'bug-11404 .rb') do |f| f.puts 'sleep'