Incorrect handling of byte-range requests ("Range:" header) #93

Closed
snej opened this Issue Oct 16, 2010 · 7 comments

Projects

None yet

2 participants

@snej

Sinatra 1.1's support for byte-range requests (the 'Range:' header) in static files is welcome, but has some serious bugs:

  • If a range is longer than 8000 bytes, the wrong number of bytes are copied from the file to the response. (In StaticFile#each, the 'break if...' statement makes no sense and screws up the loop.) This isn't caught in the unit tests in static_test.rb because they only test ranges under 100 bytes long.
  • Open-ended byte ranges like "100-" or "-100" aren't recognized at all; the Range header is ignored and the entire file gets returned.
  • Multiple ranges aren't handled correctly. The response is just a concatenation of the raw data of each range, instead of a "multipart/byteranges" structure as specified in RFC 2616.
@rkh
Sinatra member

Thanks for reporting. Will look into this tomorrow.

@snej

I don't have a patch ready yet, but the first bug can be fixed by replacing the first loop in StaticFile#each with:

 while length > 0 && (buf = read([8192,length].min))
   yield buf
   length -= buf.length
 end
@snej

Found some more areas where the implementation does not match the RFC:

  • A backwards range spec (like "100-90") should not cause a 416 response; it should just cause the Range header to be ignored.
  • An "unsatisfiable" range (one that falls off the end of the file) should cause a 416 response, instead of just returning the data to EOF.

I hope this doesn't sound snarky, but I get the impression that whoever implemented this didn't actually read the specification? RFC 2616 is pretty long, but the sections that describe byte-range requests are only two pages or so.

@rkh
Sinatra member

Removed the patch for now, so it will no longer block a release.

@snej

Re-implemented ranges and sent a pull request: http://github.com/sinatra/sinatra/pull/95

@rkh
Sinatra member

Awesome.

@snej

Re-implement byte-range support for static files.

Correct handling of "Range:" request header. Replaces buggy implementation (480b1e8, 44ab090) that was recently backed out.
Closed by 9a01e3d.

NOTE: Does not yet support multiple ranges (e.g. "bytes=1-10,20-30") because that requires sending a multipart response, which is more complex than I want to get into now.

Signed-off-by: Konstantin Haase konstantin.mailinglists@googlemail.com

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