From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS63949 64.71.152.0/24 X-Spam-Status: No, score=-2.2 required=3.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, RDNS_NONE,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from 80x24.org (unknown [64.71.152.64]) by dcvr.yhbt.net (Postfix) with ESMTP id A81251F404 for ; Sat, 21 Apr 2018 02:46:14 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] io.c: do not use rb_notify_fd_close close on recycled FD Date: Sat, 21 Apr 2018 02:46:14 +0000 Message-Id: <20180421024614.7362-1-e@80x24.org> List-Id: It is unsafe to release GVL and call rb_notify_fd_close after close(2) on any given FD. FDs (file descriptor) may be recycled in other threads immediately after close() to point to a different file description. Note the distinction between "file description" and "file descriptor". th-1 | th-2 -------------------------------+--------------------------------------- io_close_fptr | rb_notify_fd_close(fd) | fptr_finalize_flush | close(fd) | rb_thread_schedule | | fd reused (via pipe/open/socket/etc) rb_notify_fd_close(fd) | | sees "stream closed" exception | for DIFFERENT file description * thread.c (rb_thread_io_blocking_region): adjust comment for list_del * thread.c (rb_notify_fd_close): give busy list to caller * thread.c (rb_thread_fd_close): loop on busy list * io.c (io_close_fptr): do not call rb_thread_fd_close on invalid FD * io.c (io_reopen): use rb_thread_fd_close This is a partial fix for: [ruby-core:86521] [Bug #14681] Fixes: r57422 ("io.c: close before wait") --- io.c | 16 +++++++--------- test/ruby/test_io.rb | 20 ++++++++++++++++++++ thread.c | 28 ++++++++++++++++------------ 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/io.c b/io.c index ebf5743d37..dfe021af6b 100644 --- a/io.c +++ b/io.c @@ -21,6 +21,7 @@ #include #include #include "ruby_atomic.h" +#include "ccan/list/list.h" #undef free #define free(x) xfree(x) @@ -4654,15 +4655,14 @@ rb_io_memsize(const rb_io_t *fptr) # define KEEPGVL FALSE #endif -int rb_notify_fd_close(int fd); +int rb_notify_fd_close(int fd, struct list_head *); static rb_io_t * io_close_fptr(VALUE io) { rb_io_t *fptr; - int fd; VALUE write_io; rb_io_t *write_fptr; - int busy; + LIST_HEAD(busy); write_io = GetWriteIO(io); if (io != write_io) { @@ -4676,11 +4676,9 @@ io_close_fptr(VALUE io) if (!fptr) return 0; if (fptr->fd < 0) return 0; - fd = fptr->fd; - busy = rb_notify_fd_close(fd); - if (busy) { - fptr_finalize_flush(fptr, FALSE, KEEPGVL); - do rb_thread_schedule(); while (rb_notify_fd_close(fd)); + if (rb_notify_fd_close(fptr->fd, &busy)) { + fptr_finalize_flush(fptr, FALSE, KEEPGVL); /* calls close(fptr->fd) */ + do rb_thread_schedule(); while (!list_empty(&busy)); } rb_io_fptr_cleanup(fptr, FALSE); return fptr; @@ -7185,7 +7183,7 @@ io_reopen(VALUE io, VALUE nfile) rb_update_max_fd(fd); fptr->fd = fd; } - rb_notify_fd_close(fd); + rb_thread_fd_close(fd); if ((orig->mode & FMODE_READABLE) && pos >= 0) { if (io_seek(fptr, pos, SEEK_SET) < 0 && errno) { rb_sys_fail_path(fptr->pathv); diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb index 43c3ed7566..6ef1a1a6c9 100644 --- a/test/ruby/test_io.rb +++ b/test/ruby/test_io.rb @@ -3760,4 +3760,24 @@ def test_select_exceptfds con.close end end if Socket.const_defined?(:MSG_OOB) + + def test_notify_once_on_recycle + thrs = 100.times.collect do + Thread.new do + rd, wr = IO.pipe + worker = Thread.new do + sleep(0.1) + wr.syswrite('.') + end + rd.read(1) + rd.close + worker.join + wr.close + :ok + end + end + thrs.each do |t| + assert_equal :ok, t.value, '[ruby-core:86521] [Bug #14681]' + end + end end diff --git a/thread.c b/thread.c index 47b8fc23a6..881a066a92 100644 --- a/thread.c +++ b/thread.c @@ -1527,7 +1527,10 @@ rb_thread_io_blocking_region(rb_blocking_function_t *func, void *data1, int fd) } EC_POP_TAG(); - /* must be deleted before jump */ + /* + * must be deleted before jump + * this will delete either from waiting_fds or on-stack LIST_HEAD(busy) + */ list_del(&wfd.wfd_node); if (state) { @@ -2260,35 +2263,36 @@ rb_ec_reset_raised(rb_execution_context_t *ec) } int -rb_notify_fd_close(int fd) +rb_notify_fd_close(int fd, struct list_head *busy) { rb_vm_t *vm = GET_THREAD()->vm; struct waiting_fd *wfd = 0; - int busy; + struct waiting_fd *next = 0; - busy = 0; - list_for_each(&vm->waiting_fds, wfd, wfd_node) { + list_for_each_safe(&vm->waiting_fds, wfd, next, wfd_node) { if (wfd->fd == fd) { rb_thread_t *th = wfd->th; VALUE err; - busy = 1; - if (!th) { - continue; - } - wfd->th = 0; + list_del(&wfd->wfd_node); + list_add(busy, &wfd->wfd_node); + err = th->vm->special_exceptions[ruby_error_stream_closed]; rb_threadptr_pending_interrupt_enque(th, err); rb_threadptr_interrupt(th); } } - return busy; + return !list_empty(busy); } void rb_thread_fd_close(int fd) { - while (rb_notify_fd_close(fd)) rb_thread_schedule(); + LIST_HEAD(busy); + + if (rb_notify_fd_close(fd, &busy)) { + do rb_thread_schedule(); while (!list_empty(&busy)); + } } /* -- EW