valkey: accept a plus sign in big number replies and never throw on oversize ones - #39702
valkey: accept a plus sign in big number replies and never throw on oversize ones#39702robobun wants to merge 1 commit into
Conversation
…versize ones A RESP3 big number reply is `([+|-]<digits>`. The decoder only stripped `-`, so `(+42` resolved as the string "+42". It also built the BigInt with JSBigInt::stringToBigInt, which throws a RangeError when the literal has more digits than a BigInt can hold (about 313,600). That exception rejected the command. The binding now takes the digits and a sign flag and calls JSBigInt::parseInt with a null global object, so a value that does not fit comes back as the empty value instead of an exception. The decoder then falls back to the string, as it does for any other payload that is not an integer literal. The mock server in the test file now finishes a reply on drain, so a reply larger than one socket write reaches the client.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 2 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Updated 5:29 PM PT - Aug 19th, 2026
✅ @robobun, your commit 5a930991fd709d4d723b5e0226b13c6e7465a4a8 passed in 🧪 To try this PR locally: bunx bun-pr 39702That installs a local version of the PR into your bun-39702 --bun |
|
Status: the diff is complete and CI is running. Reproduced on a debug build of main (cfa9f8e) with the mock server in
With this branch both cases follow the documented rule: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and found no bugs. Because it changes a C++/Rust FFI binding signature and leans on JSBigInt::parseInt's null-globalObject behavior, a human look would still be worthwhile.
What was reviewed:
- Verified against JSC's
JSBigInt::parseInt:ParseIntSign::Signednegates the result (so passing sign-stripped digits + flag is correct), and the null-global path returnsJSValue()for oversize instead of throwing. - Confirmed the empty-digits debug
ASSERT(nullOrGlobalObjectForOOM)in JSC is guarded by the Rust-sidedigits.is_empty()check before the FFI call. - Confirmed
big_int_from_decimal's only caller is the valkey(arm, so theJsResult<Option<_>>→Option<_>signature change is contained.
Extended reasoning...
Overview
This PR fixes two edge cases in the Valkey client's RESP3 big-number (() reply decoding: a leading + sign now parses to a BigInt (matching the RESP3 spec and the : integer path), and a literal with more digits than JSC's BigInt limit (~313,600) now falls back to a string instead of rejecting the command with an uncoded RangeError. It touches src/jsc/JSValue.rs (big_int_from_decimal), the C++ binding in src/jsc/bindings/bindings.cpp + headers.h, the single caller in src/runtime/valkey_jsc/protocol_jsc.rs, docs/JSDoc, and adds three test cases plus a mock-server drain handler in test/js/valkey/valkey-incremental-scan.test.ts.
Security risks
None identified. The input is server-side wire data that was already being length-validated by the RESP parser; this change only affects how a validated line is converted to a JS value. The oversize path now degrades to a string instead of throwing, which is strictly less disruptive.
Level of scrutiny
Medium. The change is small and self-contained, but it renames and re-signatures an extern "C" binding and relies on a specific JSC internal contract (parseInt with a null globalObjectForOOM returns empty instead of throwing). I verified that contract against the vendored JSBigInt.cpp: the too-large branch checks if (nullOrGlobalObjectForOOM) before throwing and otherwise returns JSValue(), and ParseIntSign::Signed is applied as maybeResult *= -1 / tryCreateFromImpl(..., sign == Signed, ...), so stripping the sign in Rust and passing a flag is correct. I also confirmed the empty-string ASSERT at the top of parseInt cannot fire because the Rust side returns None for empty digits (and the binding ASSERT(len > 0)).
Other factors
big_int_from_decimalhas exactly one caller (grep-verified), so theJsResult<Option<JSValue>>→Option<JSValue>change and droppingfrom_js_host_callis contained. The[[ZIG_EXPORT(nothrow)]]marker matches the pattern used elsewhere (e.g.BunString.cpp) and is consistent with the Rust side no longer wrapping the call for exception propagation.- Test coverage is thorough:
(+42in both whole and byte-per-read modes,(+(sign with no digits → string), and a 400,000-digit reply → string with a follow-up PING to prove the connection survives. The mock-serverdrainhandler is a necessary addition since a 400 KB write can hit socket backpressure. - The PR description documents empirical verification with
BUN_JSC_validateExceptionChecks=1across additional edge cases ((+0,(-0,(007,(+-5, nested arrays). - Deferring rather than approving because the FFI signature/semantics change, while correct as far as I can verify, is the kind of JSC-boundary work a maintainer should sign off on.
|
The review above asks for a human look at the null global object contract. The relevant lines in the vendored JavaScriptCore, for a quick check:
The two CI build 101452 is still running, no failures so far. |
2 participants
Problem
(+42, resolves as the string"+42". The spec shape is([+|-]<digits>, and the:integer path already accepts+.big_int_from_decimal(src/jsc/JSValue.rs:676) stripped only-.(reply with more digits than a BigInt can hold (about 313,600) rejects the command with an uncodedRangeError: Out of memory: BigInt generated from this operation is too big. The binding (src/jsc/bindings/bindings.cpp:4492) calledJSBigInt::stringToBigIntwith a global object, so the size check threw. valkey: fix null array, null CRLF, big number and blob error replies #39544 documents the(arm as "integer literal becomes a BigInt, anything else stays a string, the decoder never throws". Both replies fall outside that.Fix
big_int_from_decimalstrips+or-, checks the digits, and passes the digits and a sign flag to the binding. It returnsOption<JSValue>and cannot throw.JSC__JSValue__bigIntFromDecimalDigits, callsJSBigInt::parseIntwith a null global object. In that mode JSC returns the empty value for a value that does not fit instead of an exception. This is the same call JSC's own bytecode generator uses for BigInt literals. The(arm inprotocol_jsc.rsthen takes the existing string fallback.send()JSDoc name the oversize case.test/js/valkey/valkey-incremental-scan.test.ts. On main,(+42(both send modes) and the 400,000 digit reply fail, 43 other cases pass. With the fix all 46 pass. Alsotest/integration/bun-types/bun-types.test.ts.Background
(is a big number: an integer too large for the:64-bit integer type. Redis core emits it only for values abovei64::MAX. Modules and Lua can put any text after(.JSBigInt::parseInttakes a "global object for OOM" argument. With a global object it throws a RangeError when the result would exceed JSC's BigInt limit of 2^20 bits. Withnullptrit returns the empty JSValue instead.stringToBigIntalways passes the global object.FRAMESis sent whole and one byte per read. The mock now finishes a reply ondrain, because a 400 KB reply does not fit in one socket write on every platform.Notes
JSBigInt::maxLengthBitsis 1024 * 1024.computeLengthbudgets 107/32 bits per decimal digit, so a literal with more than about 313,593 significant digits fails. The RESP line cap is 512 MB, so the gap is reachable. A 300,000 digit reply still becomes a BigInt.parseIntwith a null global asserts on an empty digit span in debug builds. The Rust side returnsNonefor an empty span before the call ((,(+and(-stay strings), and the binding assertslen > 0.BUN_JSC_validateExceptionChecks=1, no reports:(+0and(-0give0n,(007gives7n,(+-5,( 5and(0x10stay strings, a negative oversize literal stays a string, an oversize literal nested in an array stays a string next to a BigInt sibling, and PING works after each.[review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file