Fix inverted BigInt range check in JSC__isBigIntIn{Int64,UInt64}Range - #32265
Fix inverted BigInt range check in JSC__isBigIntIn{Int64,UInt64}Range#32265robobun wants to merge 4 commits into
Conversation
The (min, max) parameters were declared in the opposite order to how the Rust callers pass them, and the body returned true when the value was >= the first bound OR <= the second (the out-of-range condition). Through the MySQL parameter binder this rejected every in-range BigInt with ERR_OUT_OF_RANGE and silently accepted out-of-range ones, truncating them via to_int64().
|
Warning Review limit reached
More reviews will be available in 3 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:54 AM PT - Jun 15th, 2026
❌ @robobun, your commit af447d1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32265That installs a local version of the PR into your bun-32265 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The bot flagged #27442 as a potential duplicate. That PR has the same Happy to close this in favor of #27442 if that one is being picked back up; otherwise this is ready to go. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the inclusive range check is now correct and matches the (min, max) order used by both the Rust and Zig callers.
Extended reasoning...
Overview
This PR fixes two C++ FFI helpers, JSC__isBigIntInInt64Range and JSC__isBigIntInUInt64Range in src/jsc/bindings/bindings.cpp, which previously had swapped (max, min) parameter names relative to every caller and used an OR-based predicate that returned the inverse of the intended inclusive range check. The fix renames the parameters to (min, max) and rewrites the body to a straightforward if (x < min) return false; return x <= max; using JSBigInt::compare. A new test file test/js/sql/sql-mysql-bigint-param.test.ts exercises the MySQL parameter-binding path end-to-end against an in-process mock MySQL server, covering signed/unsigned LONGLONG boundary values and out-of-range rejections.
Verification of the fix
I confirmed all callers pass arguments as (value, min, max): the Rust wrappers at src/jsc/JSValue.rs:333/341, the Zig wrappers at src/jsc/JSValue.zig:37/41, and the downstream consumers in src/sql_jsc/mysql/MySQLValue.rs:42/45 and src/jsc/JSGlobalObject.rs:1234/1241. The new C++ signature now matches, and the comparison logic (< min → false, else != GreaterThan vs max → true) correctly implements the inclusive min <= x <= max check. JSBigInt::compare(JSBigInt*, int64_t/uint64_t) is the standard JSC overload, so no precision is lost at the boundaries.
Security risks
None. This is a numeric range-check fix in the JSC↔Rust FFI layer used only for MySQL parameter typing. If anything it tightens behavior: out-of-range BigInts that previously slipped through and were truncated on the wire are now correctly rejected with ERR_OUT_OF_RANGE. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low-to-moderate. The runtime change is ~10 lines of localized, easy-to-reason-about logic in two near-identical functions. The 250-line test file is larger but is test-only and follows the established in-process mock-MySQL-server pattern already used by ~10 sibling tests in test/js/sql/ (e.g. sql-mysql-mediumint.test.ts, sql-mysql-raw-length-prefix.test.ts, sql-mysql-auth-short-nonce.test.ts), so it is not novel infrastructure.
Other factors
No CODEOWNERS cover these paths. The bug-hunting system found no issues. The PR description includes a clear repro, root-cause analysis, and fail-before/pass-after test output. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — the new (min, max) signature now matches the Rust FFI declarations and the inclusive range check is correct; the comment-style nit can be addressed at the author's discretion.
Extended reasoning...
Overview
This PR fixes two compounding bugs in JSC__isBigIntInInt64Range / JSC__isBigIntInUInt64Range in src/jsc/bindings/bindings.cpp: the (max, min) parameter order is swapped to (min, max) to match the Rust FFI declarations in src/jsc/JSValue.rs:333/341, and the broken OR-based early-return is replaced with a straightforward inclusive min <= x <= max check. A new test/js/sql/sql-mysql-bigint-param.test.ts spins up an in-process mock MySQL server, captures COM_STMT_EXECUTE payloads, and asserts both that in-range BigInts (including i64/u64 boundaries) reach the wire with correct signedness/bytes and that out-of-range BigInts are rejected with ERR_OUT_OF_RANGE before any execute packet is sent.
I verified the Rust side independently: src/jsc/JSValue.rs declares both externs as (this, min, max) and the only call sites (src/sql_jsc/mysql/MySQLValue.rs:42/45 and src/jsc/JSGlobalObject.rs:1234/1241) all pass arguments in (min, max) order, so the new C++ signature is the correct one and there are no other callers to break.
Security risks
None. This tightens a range check so out-of-range BigInts are now rejected instead of being silently truncated and sent to the MySQL server — a strict correctness/safety improvement. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
Low-to-moderate. The C++ change is ~10 lines across two near-identical helper functions and is mechanically verifiable against JSBigInt::compare semantics (LessThan / Equal / GreaterThan). Neither modified path is covered by CODEOWNERS. The test is larger but is additive test-only code; the mock MySQL wire implementation is self-contained and only needs to be correct enough for Bun's client to complete a prepare/execute round-trip, which the passing test output in the PR description confirms.
Other factors
The bug-hunting pass surfaced only a single [Nit] about the test file's header comment narrating bug history rather than linking the issue/PR URL per CLAUDE.md conventions — purely stylistic with no functional impact. The duplicate-PR bot flagged #27442; the author has already addressed this in-thread (that PR is a stale draft against pre-rewrite paths). No outstanding human reviewer comments and no prior claude[bot] reviews on this PR.
There was a problem hiding this comment.
LGTM — the range check is now correctly min <= x <= max and matches the Rust/Zig FFI signatures; my earlier comment-style nit was addressed in dbe6a79.
Extended reasoning...
Overview
This PR fixes two compounding bugs in JSC__isBigIntInInt64Range / JSC__isBigIntInUInt64Range in src/jsc/bindings/bindings.cpp: the (min, max) parameters were declared in the wrong order relative to every caller, and the body used an OR early-return where an AND was needed. The net effect was a fully inverted range check, causing the MySQL parameter binder to reject in-range BigInts with ERR_OUT_OF_RANGE and silently truncate out-of-range ones. The fix is 6 lines per function: rename params to (min, max) and implement if (x < min) return false; return x <= max;. A new 243-line test (test/js/sql/sql-mysql-bigint-param.test.ts) spins up an in-process mock MySQL server, captures the COM_STMT_EXECUTE wire bytes, and asserts both signed/unsigned LONGLONG encoding for in-range values and ERR_OUT_OF_RANGE rejection (with no execute packet sent) for out-of-range values at all the i64/u64 boundaries.
Security risks
None introduced. The change strictly tightens validation: previously, BigInts like 2n ** 100n were accepted and sent to MySQL as a truncated 8-byte value, which is a silent data-corruption vector. The fix correctly rejects them. The mock server in the test binds to 127.0.0.1:0 and is torn down in finally.
Level of scrutiny
Low-to-moderate. The C++ change is a tiny, self-contained correctness fix to two leaf functions whose behavior is fully specified by their names. I verified the new parameter order matches both the Rust FFI declaration (src/jsc/JSValue.rs:333,341) and the Zig wrapper (src/jsc/JSValue.zig:37,41), all of which pass (this, min, max). The JSBigInt::compare semantics (compare(a, b) returns a <=> b) make the new body trivially verifiable by inspection. The test file is larger but is pure test infrastructure with no production impact; its mock-protocol implementation only needs to be correct enough for Bun's client to accept it, and the passing test run in the PR description confirms that.
Other factors
- Neither file is covered by
.github/CODEOWNERS. - My only prior feedback (replace the bug-history header comment with a PR URL per CLAUDE.md) was addressed in dbe6a79 and the inline thread is resolved; the current diff reflects the fix.
- The CI failures reported by robobun on dbe6a79 are unrelated build-infra issues (aarch64-musl build script failure and a
-no-pieunused-argument linker warning on freebsd/android-rust), not test failures; the head commit af447d1 is an explicit re-run noting "ASAN with fix passed". - A stale duplicate draft (#27442) targets pre-rewrite paths and has merge conflicts; the author has acknowledged it and this PR's test approach (Docker-free wire-level assertions) is independently preferable.
- No outstanding human reviewer comments.
|
Diff is ready. Build #62383 finished 284 passed / 2 failed, both unrelated to this change:
The new |
|
I reproduced this independently and landed on the same
pub fn is_big_int_in_int64_range(self, min: i64, max: i64) -> bool {
unsafe extern "C" {
safe fn JSC__isBigIntInInt64Range(this: JSValue, min: i64, max: i64) -> bool;
}
JSC__isBigIntInInt64Range(self, min, max)
}That restated signature is what let the argument order drift from the C++ one in the first place: both declarations are ABI-identical ( pub fn is_big_int_in_int64_range(self, min: i64, max: i64) -> bool {
crate::cpp::JSC__isBigIntInInt64Range(self, min, max)
}Same for One thing the repro turned up that may be worth an extra case in the test: My branch also carries a variant of the test built on the shared frame builders in |
…nding
The OK packet carries affected_rows and last_insert_id as u64, but both
were handed to JS through `JSValue::js_number(x as f64)`. A BIGINT
UNSIGNED AUTO_INCREMENT key above 2^53 has no exact double, so the id an
application read back from `lastInsertRowid` was not the id the server
assigned: 4611686018427387911 arrived as 4611686018427388000, silently.
Both fields now cross into JS as a number while they fit inside
Number.MAX_SAFE_INTEGER and as a BigInt past it, so the value is never
rounded. The `bigint` option is unchanged; it governs column decoding.
Binding such a key back as a query parameter is a separate bug in
JSC__isBigIntIn{Int64,UInt64}Range, already fixed in #32265.
…nding
The OK packet carries affected_rows and last_insert_id as u64, but both
were handed to JS through `JSValue::js_number(x as f64)`. A BIGINT
UNSIGNED AUTO_INCREMENT key above 2^53 has no exact double, so the id an
application read back from `lastInsertRowid` was not the id the server
assigned: 4611686018427387911 arrived as 4611686018427388000, silently.
Both fields now cross into JS as a number while they fit inside
Number.MAX_SAFE_INTEGER and as a BigInt past it, so the value is never
rounded. The `bigint` option is unchanged; it governs column decoding.
Binding such a key back as a query parameter is a separate bug in
JSC__isBigIntIn{Int64,UInt64}Range, already fixed in #32265.
|
Fresh motivation for this one, from a report that came in today.
Which lands straight on this PR: the first thing anyone does with const { lastInsertRowid } = await sql`INSERT INTO events (kind) VALUES (${"click"})`;
await sql`UPDATE events SET kind = ${"tap"} WHERE id = ${lastInsertRowid}`;
// on main: ERR_OUT_OF_RANGE, because lastInsertRowid is now a BigIntSo #33507 gives users a correct key they cannot use until this merges. I deliberately kept the I also re-derived the root cause from scratch on the current tree and got the same two defects you have: the This is still green and LGTM'd. Happy to rebase it if it's gone stale. |
…nding
The OK packet carries affected_rows and last_insert_id as u64, but both
were handed to JS through `JSValue::js_number(x as f64)`. A BIGINT
UNSIGNED AUTO_INCREMENT key above 2^53 has no exact double, so the id an
application read back from `lastInsertRowid` was not the id the server
assigned: 4611686018427387911 arrived as 4611686018427388000, silently.
Both fields now cross into JS as a number while they fit inside
Number.MAX_SAFE_INTEGER and as a BigInt past it, so the value is never
rounded. The `bigint` option is unchanged; it governs column decoding.
Binding such a key back as a query parameter is a separate bug in
JSC__isBigIntIn{Int64,UInt64}Range, already fixed in #32265.
|
Closing: this landed separately. #35246 (merged 2026-07-29) rewrote Verified on current main (bdb7382): |
1 participant
Repro
and conversely,
2n ** 100nis accepted and sent to the server as a truncated 8-byte value.Cause
JSC__isBigIntInInt64Range/JSC__isBigIntInUInt64Rangeinsrc/jsc/bindings/bindings.cpphad two compounding bugs:(value, max, min)but every Rust caller (src/jsc/JSValue.rs:333/341) passes(self, min, max), so the body saw min and max swapped.truewhenvalue >= a OR value <= b(the out-of-range condition) instead ofvalue >= min AND value <= max.Net effect: the functions returned
trueexactly when the BigInt was out of the requested range andfalsewhen it was in range. The only callers are the MySQL parameter binder (field_type_from_jsinsrc/sql_jsc/mysql/MySQLValue.rsandvalidate_big_int_rangeinsrc/jsc/JSGlobalObject.rs), so every BigInt bound as a MySQL query parameter was either spuriously rejected or silently truncated.Fix
Match the parameter order to the Rust FFI declaration and implement an inclusive
min <= x <= maxcheck: returnfalseifx < min, otherwise returnx <= max.Verification
New
test/js/sql/sql-mysql-bigint-param.test.tsruns against an in-process mock MySQL server (no Docker), captures the COM_STMT_EXECUTE payload, and asserts:5n,0n,-1n,i64::MIN,i64::MAXbind as signed LONGLONG with the exact 8-byte little-endian value on the wire.i64::MAX + 1n,u64::MAXbind as unsigned LONGLONG.u64::MAX + 1n,i64::MIN - 1n,2n ** 100n,-(2n ** 100n)reject withERR_OUT_OF_RANGEand never reach COM_STMT_EXECUTE.Fail-before on the unfixed build: the first test throws
ERR_OUT_OF_RANGEon5n; the second test showsu64::MAX + 1nreaching COM_STMT_EXECUTE with wire value0.