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

binary.toByteString() doesn't correctly handle byte arrays #439

Open
grob opened this issue Oct 28, 2021 · 1 comment
Open

binary.toByteString() doesn't correctly handle byte arrays #439

grob opened this issue Oct 28, 2021 · 1 comment

Comments

@grob
Copy link
Member

grob commented Oct 28, 2021

binary.toByteString() correctly checks if the argument is a Binary instance (which is true for ByteArray instances), but then calls toByteString() with source and target charsets. This creates a String based on the ByteArray using the source charset (default: utf-8), and a new Binary instance using the target charset (utf-8).

The effect is that stick's etag middleware (which uses binary.toByteString()) breaks responses consisting of ByteArrays, but works fine if the charset arguments in binary.toByteString() are removed.

Possible solutions: either re-apply the check introduced in 8edcb927846a in etag middleware, or remove the charset arguments here - but i'm not sure if the latter would be safe to do

@botic botic added this to the 3.0.0 milestone Nov 2, 2021
grob added a commit to ringo/stick that referenced this issue Nov 6, 2021
(see ringo/ringojs#439) to construct the etag md5 digest it copies the response body into an array of byte arrays/byte strings, but `Stream.forEach()` re-uses a single binary buffer [1], so if the streamed response exceeds 8192 bytes and thus be split up into multiple buffers the body created by etag middleware would essentially consist of different byte arrays all containing the same bytes. the fix is to use `Binary.prototype.slice()` to copy the body part - this is expensive and effectively thwarts the use of `response.stream()`, but it's what the etag middleware did before #409 too, and at least it doesn't corrupt binary responses anymore.

[1] https://github.com/ringo/ringojs/blob/master/modules/io.js#L48
@botic
Copy link
Member

botic commented Feb 22, 2022

I think we should stick to the current behavior and introduce additional checks were the type of an object is not determined.

@botic botic removed this from the 3.0.0 milestone Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants