Index out of bounds when uploading files through iOS

jwoertink commented 6 days ago

We have a small Crystal app that handles our file uploads. Our production app is sitting on Crystal 1.6.2 and works fine(ish). We've recently started upgrading to 1.12 and working out bugs and such and ran in to an issue where uploading files from iPhone generally just don't work.

When uploading a file, we get this error

Index out of bounds (IndexError)
  from /usr/share/crystal/src/slice.cr:518:5 in 'copy_to'
  from /usr/share/crystal/src/slice.cr:528:5 in 'copy_from'
  from /usr/share/crystal/src/io/delimited.cr:116:9 in 'read_with_peek'
  from /usr/share/crystal/src/io/delimited.cr:58:7 in 'read_internal'
  from /usr/share/crystal/src/io/delimited.cr:201:13 in 'read_with_peek'
  from /usr/share/crystal/src/io/delimited.cr:58:7 in 'read_internal'
  from /usr/share/crystal/src/io/delimited.cr:53:5 in 'read'
  from /usr/share/crystal/src/io.cr:286:8 in 'read_byte'
  from /usr/share/crystal/src/io.cr:396:7 in 'read_utf8_byte'
  from /usr/share/crystal/src/io.cr:363:7 in 'peek_or_read_utf8'
  from /usr/share/crystal/src/io.cr:310:13 in 'read_char_with_bytesize'
  from /usr/share/crystal/src/io.cr:309:11 in 'read_char_with_bytesize'
  from /usr/share/crystal/src/io.cr:761:14 in 'gets_slow'
  from /usr/share/crystal/src/io.cr:752:5 in 'gets_slow'
  from /usr/share/crystal/src/io.cr:662:7 in 'gets'
  from /usr/share/crystal/src/io.cr:628:5 in 'gets'
  from /usr/share/crystal/src/io.cr:598:5 in 'gets'
  from /usr/share/crystal/src/mime/multipart/parser.cr:89:13 in 'parse_headers'
  from /usr/share/crystal/src/mime/multipart/parser.cr:65:19 in '->'
  from /usr/share/crystal/src/http/server/request_processor.cr:51:20 in 'process'
  from /usr/share/crystal/src/http/server.cr:521:5 in 'handle_client'
  from /usr/share/crystal/src/http/server.cr:451:5 in '->'
  from /usr/share/crystal/src/fiber.cr:141:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:93:34 in '->'
  from ???

Works fine from desktop and Android phones, just not iPhone.

Here's a small code example that reproduces it locally.

server = HTTP::Server.new do |context|
  case context.request.path
  when "/"
    context.response.content_type = "text/html"
    context.response.print <<-HTML
      <!DOCTYPE html>
      <html>
        <body>
          <form action="/upload" method="post" enctype="multipart/form-data">
            <input type="file" name="file">
            <input type="submit" value="Upload">
          </form>
        </body>
      </html>
    HTML
  when "/upload"
    if context.request.method == "POST"
      puts "Processing file upload..."
      HTTP::FormData.parse(context.request) do |part|
        if part.name == "file"
          filename = part.filename
          if filename
            content = IO::Memory.new
            IO.copy(part.body, content)
          else
            puts "No filename provided"
          end
        end
      end
      context.response.content_type = "text/plain"
      context.response.print "File uploaded successfully"
    else
      context.response.status = HTTP::Status::METHOD_NOT_ALLOWED
    end
  else
    context.response.status = HTTP::Status::NOT_FOUND
  end
end

puts "Listening on http://0.0.0.0:8080"
server.listen "0.0.0.0", 8080

❯ crystal -v
Crystal 1.12.1 [4cea10199] (2024-04-11)

LLVM: 15.0.7
Default target: x86_64-unknown-linux-gnu

It seems that this may be related to something in IO::Delimited. I'll try and get more info on this.

/cc. @wyhaines

straight-shoota commented 6 days ago

Could you capture the HTTP traffic so we can reproduce / investigate without iOS?

wyhaines commented 6 days ago

On Tue, Jun 25, 2024 at 9:13 AM Johannes Müller @.***> wrote:

Could you capture the HTTP traffic so we can reproduce / investigate without iOS?

Possibly. I have been working on tracing down the root cause. Fundamentally, what is happening is that a file upload (any file upload) via iOS results in this line in src/io/delimited.cr failing:

@.***_delimiter_buffer[0, read_bytes])

The reason is because the slice ends up being one byte too small, and when the parser tries to handle the "\r\n" line that appears after the multipart mime headers, it can't copy two bytes into a one-byte-long slice, and it blows up.

I have a temporary work around that just inserts a size check before the copy, and reallocates a new slice that is large enough if that situation is encountered. This solves the immediate problem of not being able to upgrade the software to 1.12.2, but is itself just an interim fix.

I will work on getting a reproducible example that doesn't require possession of an iOS device. I, myself, was debugging last night with my daughter's iPhone, which wasn't ideal for either of us. :)

Kirk Haines

ysbaddaden commented 6 days ago

From that investigation it seems you can forge a request that reproduces the issue without needing an iOS device?

wyhaines commented 6 days ago

On Tue, Jun 25, 2024 at 11:52 AM Julien Portalier @.***> wrote:

From that investigation it seems you can forge a request that reproduces the issue without needing an iOS device?

That is the goal, yes. I will capture an iOS transaction that can be reproduced and used in a test case that doesn't require the device itself.

Kirk Haines

crystal-lang / crystal

Index out of bounds when uploading files through iOS #14751