Index out of bounds when uploading files through iOS about crystal

[wyhaines]

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

[straight-shoota]

I can reproduce the issue with Epiphany, which is closely related to Safari. So this might actually happen in other Safari versions as well.

It does seem to be caused by a bug in IO::Delimited related to #peek. If I disable peeking, upload works fine.

[straight-shoota]

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

[ysbaddaden]

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

[wyhaines]

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

[wyhaines]

Oh! I tried it with epiphany yesterday and it didn't fail. I will investigate more today.

…

On Tue, Jul 2, 2024, 9:55 AM Johannes Müller ***@***.***> wrote: I can reproduce the issue with Epiphany, which is closely related to Safari. So this might actually happen in other Safari version sas well. It does seem to be caused by a bug in IO::Delimited related to IO#peek. If I disable peeking, upload works fine. — Reply to this email directly, view it on GitHub <#14751 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADFQFANJG7PIWYMHZO43DZKJTIDAVCNFSM6AAAAABJ4AVLWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBSGM3TAOJQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[straight-shoota]

I don't think there's need for more investigation. You can try the patch in #14772, I expect it to fix the issue with iOS browser.

[wyhaines]

On Tue, Jul 2, 2024 at 4:21 AM Johannes Müller ***@***.***> wrote: I don't think there's need for more investigation. You can try the patch in #14772 <#14772>, I expect it to fix the issue with iOS browser.

We are testing it, but I am almost positive that patch will work. My first patch was almost exactly this, but I wasn't sure whether there was context that I was lacking around why read_bytes was being set the way that it was, so I went conservative and resorted to just bounds checking the slice itself and dealing with the issue there. I or Jeremy will confirm for you in a bit that this worked for the failure that we saw. Kirk Haines