From 3a8cadcf8f3e1c58b2c32fcd2d5a0b48cf6dfb1f Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Sun, 8 Aug 2021 18:56:16 +1200 Subject: [PATCH] Reduce chance to receive EBADF when closing an IO from another thread. --- io.c | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/io.c b/io.c index c4466581cc..bbecc427db 100644 --- a/io.c +++ b/io.c @@ -1422,6 +1422,13 @@ rb_thread_fd_writable(int fd) VALUE rb_io_maybe_wait(int error, VALUE io, VALUE events, VALUE timeout) { + // fptr->fd can be set to -1 at any time by another thread when the GVL is + // released. Many code, e.g. `io_bufread` didn't check this correctly and + // instead relies on `read(-1) -> -1` which causes this code path. We then + // check here whether the IO was in fact closed. Probably it's better to + // check that `fptr->fd != -1` before using it in syscall. + rb_io_check_closed(RFILE(io)->fptr); + switch (error) { // In old Linux, several special files under /proc and /sys don't handle // select properly. Thus we need avoid to call if don't use O_NONBLOCK. @@ -2646,31 +2653,32 @@ io_bufread(char *ptr, long len, rb_io_t *fptr) long c; if (READ_DATA_PENDING(fptr) == 0) { - while (n > 0) { + while (n > 0) { again: - c = rb_read_internal(fptr->fd, ptr+offset, n); - if (c == 0) break; - if (c < 0) { + rb_io_check_closed(fptr); + c = rb_read_internal(fptr->fd, ptr+offset, n); + if (c == 0) break; + if (c < 0) { if (fptr_wait_readable(fptr)) goto again; - return -1; - } - offset += c; - if ((n -= c) <= 0) break; - } - return len - n; + return -1; + } + offset += c; + if ((n -= c) <= 0) break; + } + return len - n; } while (n > 0) { - c = read_buffered_data(ptr+offset, n, fptr); - if (c > 0) { - offset += c; - if ((n -= c) <= 0) break; - } - rb_io_check_closed(fptr); - if (io_fillbuf(fptr) < 0) { - break; - } + c = read_buffered_data(ptr+offset, n, fptr); + if (c > 0) { + offset += c; + if ((n -= c) <= 0) break; + } + rb_io_check_closed(fptr); + if (io_fillbuf(fptr) < 0) { + break; + } } return len - n; }