Tracking Down and Fixing a Ruby Bug

The Problem

On Sunday, February 21st, @asterite tweeted this:

asterite

@asterite

Look what I found! Try this in Ruby (even in 3.0.0):

file = File.open(__FILE__)
file.each_byte do |byte|
p byte
file.close
end

13:57 PM - 21 Feb 2021

Intrigued, I threw together a little test on my laptop to confirm the bug myself:

file = File.open("/tmp/foo.txt")
file.each_byte do |byte|
  puts byte
  file.close
end

Sure enough, I got identical results to what @asterite reported in his tweet.

make: *** [uncommon.mk:842: yes-test-spec] Aborted (core dumped)

-- C level backtrace information -------------------------------------------
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(rb_print_backtrace+0x11) [0x7ffc75411dfd] vm_dump.c:758
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(rb_vm_bugreport) vm_dump.c:1042
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(rb_bug_for_fatal_signal+0xec) [0x7ffc752101fc] error.c:801
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(sigsegv+0x4d) [0x7ffc75367f2d] signal.c:960
/lib/x86_64-linux-gnu/libpthread.so.0(__restore_rt+0x0) [0x7ffc751623c0] ../sysdeps/pthread/funlockfile.c:28
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(rb_io_each_byte+0x14) [0x7ffc75262204] io.c:3993
/home/wyhaines/.ghq/github.com/wyhaines/ruby/ruby(rb_io_each_byte) io.c:3983

I followed this up by checking other IO methods which iterate over a file. A couple examples of these are as follows:

file = File.open("/tmp/foo.txt")
file.each_char do |char|
  puts char
  file.close
end

file = File.open("/tmp/foo.txt")
while byte = file.readbyte
  puts byte
  file.close
end

These all did the right thing. That is, if the filehandle was closed, even inside of a block, subsequent attempts to read from the filehandle resulted in an IOError (closed stream).

This was clearly a bug. Ruby shouldn't segmentation fault even when doing something unusual and likely wrong, like closing a filehandle inside of the block that is reading from it. With that established, the next step was to figure out what was going wrong, so that it can be fixed.

Tracking It Down

The core of Ruby is implemented in the C language, which can be somewhat beginner-unfriendly to read. However, Ruby's C is generally well structured and is written in a clear, and relatively readable style, which makes it easier to wander through the code and figure out what is going on.

The code for Ruby can be viewed or cloned from GitHub at https://github.com/ruby/ruby. Inside of the top level of the repository are a large number of C files (ending in .c). In many cases, these correspond with the classes that they implement. This is true of the io.c file, which implements the IO class. If you open it in your editor and you search for each_byte, you will find this:

each_byte

That is the implementation of the each_byte method for the class. Looking over it to get a sense of what it does, it seems fairly clear.

There is a GetOpenFile(io, fptr) call that gets a pointer to the open file handle and assign it to fptr.

Then we have two nested loops.

The outer loop...

do {
    //
    //
    //
    rb_io_check_byte_readable(fptr);
    READ_CHECK(fptr);
} while (io_fillbuf(fptr) >= 0);

...repeatedly calls io_fillbuf(fptr), which puts bytes from the IO that is being read into the file handle's read buffer until an EOF is encountered, at which time it returns a -1. This code can be found in io.c if you want to search for it and look at it. At the end of the loop block, it performs a couple of checks on the filehandle pointer, to ensure that the handle is still readable.

The inner loop...

while (fptr->rbuf.len > 0) {
    char *p = fptr->rbuf.ptr + fptr->rbuf.off++;
    fptr->rbuf.len--;
    rb_yield(INT2FIX(*p & 0xff));
    errno = 0;
}

...iterates over the filehandle's read buffer, consuming bytes until everything in the buffer has been traversed and yielding each byte, one at a time.

In that code there is a readability check, rb_io_check_byte_readable(fptr). Maybe we should learn what that does? A little searching in io.c turns up it's implementation:

void
rb_io_check_byte_readable(rb_io_t *fptr)
{
    rb_io_check_char_readable(fptr);
    if (READ_CHAR_PENDING(fptr)) {
        rb_raise(rb_eIOError, "byte oriented read for character buffered IO");
    }
}

Oh. Well, that doesn't tell us much. It mostly just calls rb_io_check_char_readable(fptr), so let's take a look at that:

void
rb_io_check_char_readable(rb_io_t *fptr)
{
    rb_io_check_closed(fptr);
    if (!(fptr->mode & FMODE_READABLE)) {
        rb_raise(rb_eIOError, "not opened for reading");
    }
    if (fptr->wbuf.len) {
        if (io_fflush(fptr) < 0)
            rb_sys_fail_on_write(fptr);
    }
    if (fptr->tied_io_for_writing) {
        rb_io_t *wfptr;
        GetOpenFile(fptr->tied_io_for_writing, wfptr);
        if (io_fflush(wfptr) < 0)
            rb_sys_fail_on_write(wfptr);
    }
}

Right at the top of that, there is another call to another function that might be worth knowing about, rb_io_check_closed(fptr).

Following the bread crumbs leads to this

void
rb_io_check_closed(rb_io_t *fptr)
{
    rb_io_check_initialized(fptr);
    io_fd_check_closed(fptr->fd);
}

That seems pretty straightforward. More breadcrumbs can be followed, but it sure looks like that checks to see if the filehandle pointer has been initialized, and then it does a check to see if the device that the filehandle points to has been closed.

To confirm that supposition, let's find io_fd_check_closed():

static void
io_fd_check_closed(int fd)
{
    if (fd < 0) {
        rb_thread_check_ints(); /* check for ruby_error_stream_closed */
        rb_raise(rb_eIOError, closed_stream);
    }
}

If the filehandle has been closed, this raises the IOError(closed stream) error.

Figuring It Out

So, at this point, there is confirmation that the each_byte implementation does have a check to see if the filehandle has been closed, and that check will throw an exception if it has been. However, we can see from the test script that at the beginning of this article that this is not working, so let's look at the whole function again:

static VALUE
rb_io_each_byte(VALUE io)
{
    rb_io_t *fptr;

    RETURN_ENUMERATOR(io, 0, 0);
    GetOpenFile(io, fptr);

    do {
        while (fptr->rbuf.len > 0) {
            //rb_io_check_byte_readable(fptr);
            char *p = fptr->rbuf.ptr + fptr->rbuf.off++;
            fptr->rbuf.len--;
            rb_yield(INT2FIX(*p & 0xff));
            errno = 0;
        }
        rb_io_check_byte_readable(fptr);
        READ_CHECK(fptr);
    } while (io_fillbuf(fptr) >= 0);
    return io;
}

What happens if the filehandle is closed while the code inside the block that is yielded to (rb_yield(INT2FIX(*p & 0xff))) is executing?

If there are still bytes to read, the block inside of the while loop will be executed again. It will attempt to set the char pointer *p to the next byte in the buffer. But if the filehandle has been closed, then the data structure that fptr points at will have been freed, and trying to access a data pointer that has been freed results in a segmentation fault.

To quote Wikipdeia:

Segmentation faults are a common class of error in programs written in languages like C that provide low-level memory access. They arise primarily due to errors in use of pointers for virtual memory addressing, particularly illegal access.

Ah-ha!

The root of the bug really is very simple. The check to ensure that the filehandle remains readable is occurring outside of where it is most likely -- which is inside of the inner loop. It needs to be checked before the code tries to access the filehandle's read buffer.

If a check is inserted at that point:

while (fptr->rbuf.len > 0) {
    rb_io_check_byte_readable(fptr);
    char *p = fptr->rbuf.ptr + fptr->rbuf.off++;
    fptr->rbuf.len--;
    rb_yield(INT2FIX(*p & 0xff));
    errno = 0;
}

Then if the filehandle is closed prior to the attempt to access a byte from it, the closure will be detected, and an exception raised.

We can test this by adding a spec to the spec/ruby/core/io/each_byte_spec.rb file, which tests the behavior of IO#each_byte.

it "raises IOError when stream is closed mid-read" do
  -> do
    @io.each_byte do |byte|
      @io.close
    end
  end.should raise_error(IOError)
end

Running this on a Ruby without the above change results in a segmentation fault, just like that which is received from the original test program.

Running the specs on a Ruby with the above change, though, has a much better outcome:

Finished in 0.004663 seconds

1 file, 6 examples, 7 expectations, 0 failures, 0 errors, 0 tagged

It turns out that this bug has existed for a very long time. Using the time machine that is git, one can go back to the Ruby 1.9.1 branch:

for (;;) {
    p = fptr->rbuf+fptr->rbuf_off;
    e = p + fptr->rbuf_len;
    while (p < e) {
        fptr->rbuf_off++;
        fptr->rbuf_len--;
        rb_yield(INT2FIX(*p & 0xff));
        p++;
        errno = 0;
    }
    rb_io_check_readable(fptr);
    READ_CHECK(fptr);
    if (io_fillbuf(fptr) < 0) {
        break;
    }
}

The loop implementation was a bit different at this time, but the main details are the same, and you can see that the check for readability had been moved outside of the inner buffer-walking loop as many as 14 years ago.

While this bug was very simple to hunt down, it worked very well to show the iterative steps that can be used with the Ruby codebase to research, identify, isolate, and fix other bugs. The Ruby Issue Tracking System is viewable by anyone. If you are curious, go take a look at it. You may see something that you can track down and fix, too.