Skip to content
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

Fix static file handler for range requests on files > 2^31 bytes #546

Merged
merged 1 commit into from
Aug 3, 2019
Merged

Fix static file handler for range requests on files > 2^31 bytes #546

merged 1 commit into from
Aug 3, 2019

Conversation

omarroth
Copy link
Contributor

To reproduce on master:

require "kemal"
Kemal.run

Serving a file > ~2.1 GB, for example:

$ dd if=/dev/urandom of=file2 bs=1000 count=2147484 status=progress

Request with:

$ curl http://localhost:3000/file2 -H 'Range: bytes=0-' -v -o /dev/null

which produces:

Exception: Negative limit (ArgumentError)
  from /usr/lib/crystal/io.cr:1147:5 in 'copy'
  from /helpers.cr:218:5 in 'multipart'
  from src/kemal/helpers/helpers.cr:137:12 in 'send_file'
  from src/kemal/static_file_handler.cr:65:9 in 'call'
  from /usr/lib/crystal/http/server/handler.cr:26:7 in 'call_next'
  from src/kemal/exception_handler.cr:8:7 in 'call'
  from /usr/lib/crystal/http/server/handler.cr:26:7 in 'call_next'
  from src/kemal/log_handler.cr:8:37 in 'call'
  from /usr/lib/crystal/http/server/handler.cr:26:7 in 'call_next'
  from src/kemal/init_handler.cr:12:7 in 'call'
  from /usr/lib/crystal/http/server/request_processor.cr:39:11 in 'process'
  from /usr/lib/crystal/http/server/request_processor.cr:16:3 in 'process'
  from /usr/lib/crystal/http/server.cr:462:5 in 'handle_client'
  from /usr/lib/crystal/http/server.cr:428:13 in '->'
  from /usr/lib/crystal/fiber.cr:255:3 in 'run'
  from /usr/lib/crystal/fiber.cr:47:34 in '->'
  from ???

2019-07-31 18:36:19 UTC 500 GET /file2 112.57ms

Description

The static file handler will serve an incorrect content length when encountering a range request on files > 2^31 bytes (Int32::MAX) due to an integer overflow.

Since file.size returns a UInt64, I've updated the rest of the code to use the same type.

@straight-shoota
Copy link
Contributor

It's better to use Int64 for math operation to avoid negative overflow. Int64::MAX should still be plenty of range.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @omarroth 🙏

@sdogruyol sdogruyol merged commit 18ddc3b into kemalcr:master Aug 3, 2019
@omarroth omarroth deleted the fix-static-handler branch August 5, 2019 02:03
env.response.status_code = 206
env.response.content_length = content_length
env.response.headers["Accept-Ranges"] = "bytes"
env.response.headers["Content-Range"] = "bytes #{startb}-#{endb}/#{fileb}" # MUST

if startb > 1024
skipped = 0
skipped = 0_i64
# file.skip only accepts values less or equal to 1024 (buffer size, undocumented)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file.skip only accepts values less or equal to 1024 (buffer size, undocumented)

@sdogruyol Do you know what this means? I tried it and it works fine with values greater than1024.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been a long time since I touched this..seems like the statement is wrong as of now 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants