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

The number -2^53 should be encoded as a float, not an integer #155

Closed
joeltg opened this issue Sep 16, 2021 · 3 comments
Closed

The number -2^53 should be encoded as a float, not an integer #155

joeltg opened this issue Sep 16, 2021 · 3 comments
Assignees

Comments

@joeltg
Copy link

joeltg commented Sep 16, 2021

This is a niche issue relating to just the value - 2^53

The CBOR spec, in the section on converting JSON to CBOR, says

JSON numbers without fractional parts (integer numbers) are represented as integers (major types 0 and 1, possibly major type 6, tag number 2 and 3), choosing the shortest form; integers longer than an implementation-defined threshold may instead be represented as floating-point values. The default range that is represented as integer is -2^53+1..2^53-1

And sure enough, there's a check on this line to make sure that integers greater than Number.MAX_SAFE_INTEGER get encoded as floats, not as integer types. However this check happens after negative integer values are mapped to positive integer values. This means there's an offset of 1 for negative integers that isn't accounted for when checking if they're safe or not.

For example the safe integer Number.MIN_SAFE_INTEGER gets mapped to - Number.MIN_SAFE_INTEGER - 1 = 9007199254740990 = Math.pow(2, 53) - 2, which node-cbor correctly encodes as a major type 1 with a uint64 argument. But the unsafe integer Number.MIN_SAFE_INTEGER - 1 gets mapped to - (Number.MIN_SAFE_INTEGER - 1) - 1 = 9007199254740991 = Math.pow(2, 53) - 1, which node-cbor thinks is a safe integer value since it's less than or equal to Number.MAX_SAFE_INTEGER.

Hopefully that made sense. Concretely, these three numbers encode as expected:

cbor.encode(Number.MAX_SAFE_INTEGER)
// <Buffer 1b 00 1f ff ff ff ff ff ff>
// major type 0, uint64 argument 2^53 - 1

cbor.encode(Number.MAX_SAFE_INTEGER + 1)
// <Buffer fa 5a 00 00 00>
// major type 7, float32 with exponent 53 and no precision bits

cbor.encode(Number.MIN_SAFE_INTEGER)
// <Buffer 3b 00 1f ff ff ff ff ff fe>
// major type 1 

But this behavior is not expected:

cbor.encode(Number.MIN_SAFE_INTEGER - 1)
// <Buffer 3b 00 1f ff ff ff ff ff ff>
// this number is below the min safe integer range and should be kept as a float!
// we can't trust the precision of this number - for example the user may have
//  "started with" -2^53-1, which is unrepresentable and rounds up to -2^53
@hildjj
Copy link
Owner

hildjj commented Sep 16, 2021

willfix, thanks for the clear writeup.

@hildjj hildjj self-assigned this Sep 16, 2021
@hildjj
Copy link
Owner

hildjj commented Sep 29, 2021

I left a couple of comments on the PR for you to double-check my work, if you please. Sorry the patch is so large, I wanted to regen docs etc so I'm ready to do a release once you sign off.

@hildjj hildjj closed this as completed in 8ca67dc Sep 29, 2021
hildjj added a commit that referenced this issue Sep 29, 2021
@hildjj
Copy link
Owner

hildjj commented Sep 29, 2021

Fixed in 8.0.2

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

2 participants