The Relicans

Kirk Haines
Kirk Haines

Posted on • Updated on

Tracking Down and Fixing a Ruby Bug

The Problem

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

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
Enter fullscreen mode Exit fullscreen mode

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

make: *** [uncommon.mk:842: yes-test-spec] Aborted (core dumped)
Enter fullscreen mode Exit fullscreen mode
-- 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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode
file = File.open("/tmp/foo.txt")
while byte = file.readbyte
  puts byte
  file.close
end
Enter fullscreen mode Exit fullscreen mode

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

image

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);
Enter fullscreen mode Exit fullscreen mode

...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;
}
Enter fullscreen mode Exit fullscreen mode

...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");
    }
}
Enter fullscreen mode Exit fullscreen mode

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);
    }
}
Enter fullscreen mode Exit fullscreen mode

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);
}
Enter fullscreen mode Exit fullscreen mode

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);
    }
}
Enter fullscreen mode Exit fullscreen mode

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;
}
Enter fullscreen mode Exit fullscreen mode

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;
}
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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;
    }
}
Enter fullscreen mode Exit fullscreen mode

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.


I stream on Twitch for The Relicans. Stop by and follow me at https://www.twitch.tv/wyhaines, and feel free to drop in any time. In addition to whatever I happen to be working on that day, I'm always happy to field questions or to talk about anything that I may have written.

Discussion (0)