hildjj / node-cbor

Encode and decode CBOR documents, with both easy mode, streaming mode, and SAX-style evented mode.
MIT License
356 stars 73 forks source link

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

Closed joeltg closed 2 years ago

joeltg commented 2 years ago

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 commented 2 years ago

willfix, thanks for the clear writeup.

hildjj commented 2 years ago

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 commented 2 years ago

Fixed in 8.0.2