socketry / async-redis

MIT License
83 stars 18 forks source link

Add failing test with a large response from Redis #6

Closed davidor closed 5 years ago

davidor commented 5 years ago

I added a test that shows that LRANGE crashes when it receives a large response from Redis as discussed in #4

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 65


Totals Coverage Status
Change from base Build 62: 0.5%
Covered Lines: 315
Relevant Lines: 374

💛 - Coveralls
davidor commented 5 years ago

I think that I've found the problem. It's caused by how the read_line function in Async::IO::Protocol::Line works.

Here it is: https://github.com/socketry/async-io/blob/bb6f3240dcebab448eb2cc2ff1893ad714e8116e/lib/async/io/stream.rb#L87

After reading some elements of the response, I observed that the read buffer contained just \r. When calling read_line, the method does not find \r\n in the buffer, updates the offset with the buffer size (which is 1), and the reads another BUFFER_SIZE bytes. The problem is that the code should not update the offset that way. It's discarding a byte that can be part of the pattern.

So for example, imagine that we have \r in the read buffer, and after reading another BUFFER_SIZE bytes we have \r\n$4\r\n1234..... The code discards \r\n$4 and then, at some point it will return '1' as a token, which is invalid.

I think this could fix the issue, but I'm not sure whether it could affect other functionality:

def read_until(pattern, offset = 0, chomp: true)
  until index = @read_buffer.index(pattern, offset)
    offset += [0, @read_buffer.size - (pattern.bytesize - 1)].max ## fix is here
    return unless fill_read_buffer
end

Let me know what you think @ioquatix. I can try to add the fix in the async-io and some tests if you think that this is the correct approach to fix the bug.

ioquatix commented 5 years ago

Can you please make a PR on async-io along with a spec if possible. Yes, you appear to have done a fantastic job of identifying the issue.

ioquatix commented 5 years ago

I am going to take a stab at this.