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 handling of codepoints >= 0xe000 #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

svaarala
Copy link

@svaarala svaarala commented Jun 7, 2019

Codepoints in [U+E000,U+FFFF] were incorrectly handled by the surrogate pair code path.

Example of behavior before the fix:

duk> CBOR.encode('\ue000')
= |64f0908080|
duk> CBOR.decode(CBOR.encode('\ue000'))
= "\ud800\udc00"
duk> CBOR.decode(CBOR.encode('\uffff'))
= "\udbff\udc00"

After the fix:

duk> CBOR.encode('\ue000')
= |63ee8080|
duk> CBOR.decode(CBOR.encode('\ue000'))
= "\ue000"
duk> CBOR.decode(CBOR.encode('\uffff'))
= "\uffff"

Cbor.me gives the following for encoding "\ue000", matching the fixed output:

63        # text(3)
   EE8080 # "\xEE\x80\x80"

Codepoints in [U+E000,U+FFFF] were handled by the surrogate pair
code path.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.262% when pulling 0f1ed4a on svaarala:fix-cp-above-dfff into 65dc496 on paroga:master.

@TrevorFSmith
Copy link

TrevorFSmith commented Aug 17, 2019

This bug is the cause of failure to properly roundtrip emoji with modifiers such as ☝️ and 👆🏿, which are both U+261D with modifier️s U+FE0F or U+1F3FF respectively.

When I apply this change the en/decode roundtrip is successful. Without the change it fails to encode the modifier.

Example code:
let value = '_☝️_👆🏿_'; console.log(value, cbor.decode(cbor.encode(value)));
Without fix: Screen Shot 2019-08-17 at 11 35 00 AM

With fix: Screen Shot 2019-08-17 at 11 35 32 AM

@TrevorFSmith
Copy link

@paroga It looks like this lib needs some love. Would you be up for handing the reins to someone else either temporarily or permanently? I'd be happy to review the existing PRs and Issues, either in this repo or (if you'd prefer) in a repo under my work org.

@svaarala
Copy link
Author

I'd also be happy to help in whatever way I can. I'm working with Duktape's native CBOR binding so I could help at least with bug fixing and maybe improve test coverage (Duktape's CBOR binding is based on this library, so many of the testcases in Duktape repo can be used as is).

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.

3 participants