New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileBufferedChannel race condition #1326

Closed
FooBarWidget opened this Issue Dec 18, 2014 · 2 comments

Comments

Projects
None yet
1 participant
@FooBarWidget
Member

FooBarWidget commented Dec 18, 2014

This was reported by Daniel Shannon. See the attached crash logs:
https://gist.github.com/FooBarWidget/8626b268c8f69c59bceb
https://gist.github.com/FooBarWidget/3d17bc10e5023d22f897

According to Daniel, Passenger seems to crash whenever he tries to access one of the applications, but only if he accesses from iPhone, and only when the server is serving a pair of video files. This might be some kind of race condition caused by network latency.

I've been able to reproduce the issue by serving two large Rack responses at the same time to clients that don't read the response body. Here's what the server looks like:

class LargeBody
  def each
    kb = "x" * 1024
    (80 * 1024).times do
      yield kb
    end
  end
end

I then run two clients:

require 'socket'
sock = TCPSocket.new('127.0.0.1', 3000)
sock.write("GET / HTTP/1.1\r\n")
sock.write("\r\n")
STDIN.readline

My crash log is here: https://gist.github.com/FooBarWidget/ca9958d607e2fbe0f7bb

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Dec 18, 2014

Member

This has been identified as a race condition with libeio. Consider this code:

moveContext->req = eio_write(inFileMode->fd,
    moveContext->buffer.start,
    inFileMode->readOffset + inFileMode->written,
    0, _bufferWrittenToFile, moveContext);

eio_write() passes the I/O request to a thread before returning. This thread might finish (and call the callback) before the moveContext->req = assignment finishes. The callback tries to use moveContext->req, observes a NULL pointer and crashes.

We need to find a way to start an I/O request without dispatching it to a thread yet.

Member

FooBarWidget commented Dec 18, 2014

This has been identified as a race condition with libeio. Consider this code:

moveContext->req = eio_write(inFileMode->fd,
    moveContext->buffer.start,
    inFileMode->readOffset + inFileMode->written,
    0, _bufferWrittenToFile, moveContext);

eio_write() passes the I/O request to a thread before returning. This thread might finish (and call the callback) before the moveContext->req = assignment finishes. The callback tries to use moveContext->req, observes a NULL pointer and crashes.

We need to find a way to start an I/O request without dispatching it to a thread yet.

@FooBarWidget FooBarWidget changed the title from Passenger 5 crashes to FileBufferedChannel race condition Dec 18, 2014

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jan 2, 2015

Member

There is another problem. context->req is mutable, and may even be modified from another thread. The current cancel() implementation is therefore unsafe.

Member

FooBarWidget commented Jan 2, 2015

There is another problem. context->req is mutable, and may even be modified from another thread. The current cancel() implementation is therefore unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment