Repository navigation
fix: correct carry for large negative 64-bit values - #215
Open
Brijesh-Thakkar wants to merge 1 commit into
Open
Brijesh-Thakkar wants to merge 1 commit into
Brijesh-Thakkar wants to merge 1 commit into
Conversation
LongBits.fromBigInt negated the magnitude word by word with BigInts, where ~0n + 1n is 0n rather than 2^32, so the carry into the high word was never taken. Negative values below -(2^53 - 1) whose low 32 bits are zero (e.g. -2^53, -2^60, INT64_MIN) were encoded as value - 2^32 by int64, sint64 and sfixed64. Take the two's complement with BigInt.asUintN(64, value) instead, and add regression tests to protons-runtime along with its test scripts. Fixes ipfs#214
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #214
What's wrong
LongBits.fromBigIntnegates the magnitude word by word using BigInts:BigInts don't wrap at 32 bits. When the low word is
0n,~0n + 1nis0n, not2^32, so the carry intohiis never taken. Every negative value belowNumber.MIN_SAFE_INTEGERwhose low 32 bits are zero is affected, for example −2^53, −2^60, −(2^63 − 2^32) andINT64_MIN. These encode asvalue − 2^32inint64,sint64andsfixed64. The output is valid protobuf, so nothing flags it, and a round trip silently returns the wrong value. Values inside the safe-integer range take thefromNumberpath and are unaffected, and so is decoding.This is the "separate overflow in
fromBigInt" noted in #212. It covers more thanINT64_MIN.Minimal repro
Fix
Take the two's complement in one step:
TWO_32was only used there, so it's removed.Tests
protons-runtime didn't have its own test suite yet, so this adds
packages/protons-runtime/test/longbits.spec.tsplus the standard aegirtest*scripts to itspackage.json. The new tests coverint64,sint64andsfixed64for 5 affected values and 1 control (−(2^53 + 1)), checking both the exact encoded bytes and the round trip. The expected bytes come from an independent BigInt encoder written from the protobuf spec. If you'd rather keep these next to the #212 tests inpackages/protons/test/longbits.spec.ts, I'm happy to move them.Local results (Node v22.23.1, Linux x64):
--bail)npm run test:node(all packages)test:chrome,test:chrome-webworker,test:firefox,test:firefox-webworkertest:electron-mainbuild,lint,dep-check,doc-check,spell-checkRelease note
The 8.0.2 release from #212 hasn't reached npm yet: the tag and release commit exist, but run 38750177343 failed before publishing. If 8.0.2 is going to be re-released, it would be good to include this fix alongside #212, since both are 64-bit encoding fixes in
LongBitsthat have been present since 5.0.4.