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

Fails to encode strings containing non-ASCII characters #1

Open
sorear opened this issue Mar 2, 2013 · 3 comments
Open

Fails to encode strings containing non-ASCII characters #1

sorear opened this issue Mar 2, 2013 · 3 comments

Comments

@sorear
Copy link

sorear commented Mar 2, 2013

The browser encoder uses character length for strings, but the browser decoder, node encoder, and node decoder all expect byte lengths. This leads to hilarity at decode time.

@ericz
Copy link
Member

ericz commented Apr 3, 2013

Yes indeed. It's just hard to get byte lengths on client side in a performant manner. I'll probably make a UTF happy fork sometime by using (new Blob([str])).size instead of str.length sometime, but I don't want binarypack to be slow

@dnorman
Copy link

dnorman commented Apr 3, 2013

I would agree that performance is important, but this malfunctions in a particularly egregious way when UTF8 data is passed, which is to say that it's quite vulnerable, as UTF8 is ever-present on the modern web. Notwithstanding this bug, binaryjs is an excellent library btw.

@ericz
Copy link
Member

ericz commented Apr 3, 2013

I see what you're saying.

Any suggestion for solutions?

Options:

  • Take a performance hit and get string lengths with (new Blob([str])).size instead of str.length
  • Offer a UTF8 flag / fork that takes the performance hit if people so desire
  • Do nothing

Or other ideas?

ericz pushed a commit that referenced this issue Dec 19, 2013
Fix a bug when calculating the length of string containing non-ASCII characters
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

No branches or pull requests

3 participants