sql: purify NaN before boxing binary float8/double values from the wire - #33824
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (2)
Comment |
|
Updated 3:32 AM PT - Jul 9th, 2026
❌ @robobun, your commit 46969f3 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33824That installs a local version of the PR into your bun-33824 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line purifyNaN() wrap matching the established pattern from #32787/#33072, with a hermetic wire-protocol test.
Extended reasoning...
Overview
The production change is a single token: jsNumber(cell.value.number) → jsNumber(purifyNaN(cell.value.number)) in src/jsc/bindings/SQLClient.cpp for the DataCellTag::Double case. purifyNaN is the canonical JSC helper that is identity for non-NaN doubles and collapses any NaN bit pattern to JSC's single canonical NaN, preventing tag-range collisions in JSVALUE64 NaN-boxing. The accompanying test (test/js/sql/postgres-binary-float-nan-box.test.ts) uses the existing test/js/sql/wire-frames.ts mock-server helpers to feed three forged NaN payloads plus a control value (3.5) through the real client path.
Security risks
This closes a type-confusion / forged-JSValue hole where untrusted wire bytes could produce arbitrary JSValues (including cell pointers → segfault). The fix cannot introduce a new risk: purifyNaN only alters values that are already NaN, and the only observable change is that non-canonical NaNs now surface as the JS number NaN instead of a mistagged value. There is no auth, crypto, or permissions logic touched.
Level of scrutiny
Low. This is the third application of the identical fix in this codebase — the same wrap already landed in napi.cpp (#32787) and V8Number.cpp (#33072), and purifyNaN is also used in SerializedScriptValue.cpp and JSDOMConvertNumbers.h. The pattern is well-understood and the diff is mechanical. No CODEOWNERS entry covers these paths.
Other factors
The test is hermetic (local net server via the shared listeningServer/pg* frame builders, no external hosts), cleans up via await using + server.close() in finally, uses test.each for the payload matrix, and includes a positive control asserting non-NaN doubles round-trip unchanged. All imported helpers exist in test/js/sql/wire-frames.ts. The bug-hunting system found no issues, and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — one-line purifyNaN wrap mirroring the identical fix already landed in napi.cpp and V8Number.cpp, with a wire-level regression test.
Extended reasoning...
Overview
This PR wraps cell.value.number in purifyNaN() before passing it to jsNumber() in src/jsc/bindings/SQLClient.cpp (DataCellTag::Double case). It adds a regression test at test/js/sql/postgres-binary-float-nan-box.test.ts that stands up a mock Postgres wire server (via the existing test/js/sql/wire-frames.ts helper) and returns forged NaN bit patterns in a binary float8 column, asserting they decode as NaN rather than forged JSValues, plus a control case verifying an ordinary double (3.5) round-trips unchanged.
Security risks
The PR fixes a security issue (untrusted wire bytes → non-canonical NaN → forged JSValue via NaN-boxing collision, potentially a crash or worse via forged cell pointers). The fix itself introduces no new attack surface: purifyNaN is a pure, side-effect-free canonicalizer that is a no-op on non-NaN inputs and collapses any NaN payload to JSC's single canonical NaN. There is no auth, crypto, or permissions logic involved.
Level of scrutiny
Low. This is a mechanical, one-line application of an established in-tree pattern — the exact same jsNumber(purifyNaN(x)) idiom already appears in napi.cpp, v8/V8Number.cpp, SerializedScriptValue.cpp, and JSDOMConvertNumbers.h for the same reason (boxing an untrusted double). The PR description explicitly cites #32787 and #33072 as prior instances of the same root-cause fix, and notes #32787 called out the SQL decoders as a known deferred gap. There is no design ambiguity here.
Other factors
- The test imports (
listeningServer,pgAuthenticationOk,pgReadyForQuery,pgRowDescription,pgDataRow,pgCommandComplete) all resolve against the existingtest/js/sql/wire-frames.tsharness. - The test includes a positive control (non-NaN 3.5) so a regression that over-purifies would be caught.
- The bug-hunting system found no issues.
- No prior reviewer comments or outstanding requests on the PR timeline.
|
The diff is green: the new test `test/js/sql/postgres-binary-float-nan-box.test.ts` passed (4 pass, 0 fail) on the darwin x64 lane, and the change is a single `purifyNaN` wrap in `SQLClient.cpp`. The two red tests on that lane are unrelated to this change:
Neither touches the SQL decoders or NaN-boxing. Ready for a maintainer to merge. |
… forge a JSValue (#35435) ## What `File`/`Blob` store `lastModified` as a raw `f64`. The `lastModified` getter boxed that value with `JSValue::js_number(...)` without purifying NaN. The structured-clone deserialize path (`read_float`, a plain `f64::from_ne_bytes`) reads the value straight off the wire, so a crafted `v8.deserialize` / `bun:jsc.deserialize` buffer could plant an impure NaN. JSVALUE64 boxing is only sound for a non-NaN or the single canonical NaN. Any other NaN bit pattern collides with the tag ranges for `boolean`, `undefined`, `Int32`, and cell pointers, so `.lastModified` (a property that must always be a `Number`) decoded as a forged immediate or a forged cell pointer that segfaults when dereferenced. The constructor already mapped `NaN` to `0` (#33922), and the generic structured-clone number path already purifies (`jsNumber(purifyNaN(d))`), but the custom `File`/`Blob` tag getter skipped it. This is the same bug class and primitive as #33823/#33824 (`bun:sql` `float8`) in a sink those fixes did not cover. ## Repro (pre-fix) ```js import { serialize, deserialize } from "bun:jsc"; const sentinelBE = Buffer.from([0x3a, 0x1b, 0x2c, 0x3d, 0x4e, 0x5f, 0x60, 0x71]); const sentinel = sentinelBE.readDoubleBE(0); const sentinelLE = Buffer.alloc(8); sentinelLE.writeDoubleLE(sentinel, 0); const buf = Buffer.from(serialize(new File(["x"], "a.txt", { lastModified: sentinel }))); const at = buf.indexOf(sentinelLE); Buffer.from("0810000000feffff", "hex").copy(buf, at); // forged unmapped cell pointer const forged = deserialize(buf).lastModified; Object.prototype.toString.call(forged); // SEGV ``` ``` panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF ``` Under a debug + ASAN build the impure NaN trips `ASSERTION FAILED: !isImpureNaN(d)` at the moment `js_number` boxes it. ## Fix Purify every NaN at both `lastModified` boxing sites in `src/runtime/webcore/Blob.rs`, mirroring the sink-level fix used in #32787 and #33824. `JSValue::purify_nan` already exists. A deserialized `.lastModified` now always reads back as a `Number` (canonical NaN for a planted NaN), never a forged immediate or cell. ## Verification - New test in `test/js/web/structured-clone-blob-file.test.ts` forges `boolean`, `undefined`, `int32`, and cell-pointer payloads through both `bun:jsc.deserialize` and `v8.deserialize`, asserting each comes back as `number` NaN. It runs in a child process since the pre-fix build crashes. - Fails on the unfixed build (child aborts at the `!isImpureNaN` assert before producing output), passes with the fix. Closes #35434 <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/structured-clone-blob-file.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… forge a JSValue (#35435) ## What `File`/`Blob` store `lastModified` as a raw `f64`. The `lastModified` getter boxed that value with `JSValue::js_number(...)` without purifying NaN. The structured-clone deserialize path (`read_float`, a plain `f64::from_ne_bytes`) reads the value straight off the wire, so a crafted `v8.deserialize` / `bun:jsc.deserialize` buffer could plant an impure NaN. JSVALUE64 boxing is only sound for a non-NaN or the single canonical NaN. Any other NaN bit pattern collides with the tag ranges for `boolean`, `undefined`, `Int32`, and cell pointers, so `.lastModified` (a property that must always be a `Number`) decoded as a forged immediate or a forged cell pointer that segfaults when dereferenced. The constructor already mapped `NaN` to `0` (#33922), and the generic structured-clone number path already purifies (`jsNumber(purifyNaN(d))`), but the custom `File`/`Blob` tag getter skipped it. This is the same bug class and primitive as #33823/#33824 (`bun:sql` `float8`) in a sink those fixes did not cover. ## Repro (pre-fix) ```js import { serialize, deserialize } from "bun:jsc"; const sentinelBE = Buffer.from([0x3a, 0x1b, 0x2c, 0x3d, 0x4e, 0x5f, 0x60, 0x71]); const sentinel = sentinelBE.readDoubleBE(0); const sentinelLE = Buffer.alloc(8); sentinelLE.writeDoubleLE(sentinel, 0); const buf = Buffer.from(serialize(new File(["x"], "a.txt", { lastModified: sentinel }))); const at = buf.indexOf(sentinelLE); Buffer.from("0810000000feffff", "hex").copy(buf, at); // forged unmapped cell pointer const forged = deserialize(buf).lastModified; Object.prototype.toString.call(forged); // SEGV ``` ``` panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF ``` Under a debug + ASAN build the impure NaN trips `ASSERTION FAILED: !isImpureNaN(d)` at the moment `js_number` boxes it. ## Fix Purify every NaN at both `lastModified` boxing sites in `src/runtime/webcore/Blob.rs`, mirroring the sink-level fix used in #32787 and #33824. `JSValue::purify_nan` already exists. A deserialized `.lastModified` now always reads back as a `Number` (canonical NaN for a planted NaN), never a forged immediate or cell. ## Verification - New test in `test/js/web/structured-clone-blob-file.test.ts` forges `boolean`, `undefined`, `int32`, and cell-pointer payloads through both `bun:jsc.deserialize` and `v8.deserialize`, asserting each comes back as `number` NaN. It runs in a child process since the pre-fix build crashes. - Fails on the unfixed build (child aborts at the `!isImpureNaN` assert before producing output), passes with the fix. Closes #35434 <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/structured-clone-blob-file.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Fixes #33823
Problem
bun:sqlboxes the raw 8 bytes of a binaryfloat8/DOUBLEcolumn straight into a JSValue. A tagged-template or parameterized query automatically requests binary result format for numeric columns, so the bytes come from whatever sits on the other end of the socket (the database, a proxy, or a MITM on a plaintext connection).jsNumber(double)NaN-boxes by addingDoubleEncodeOffset. JSC's JSVALUE64 encoding is only sound when the boxed double is a non-NaN or the single canonical purified NaN. Any other NaN bit pattern collides with the tag ranges JSC uses for booleans,undefined, Int32, and cell pointers, so a server can make adouble precisioncolumn come back as an arbitrary forged JSValue, or crash the process with a forged cell pointer.This is the same root cause already fixed for
bun:ffi/N-API in #32787 and forv8::Number::Newin #33072; that first PR explicitly called out the SQL decoders as a known, deferred gap.Cause
src/jsc/bindings/SQLClient.cpp,DataCellTag::Double:return jsNumber(cell.value.number);cell.value.numberis produced byf64::from_bits(...)over the raw wire bytes (Postgresparse_binary_float8, MySQLMYSQL_TYPE_DOUBLE/FLOAT), with no validation or purification in between. Both producers feed this one sink.Fix
Purify the NaN at the sink, mirroring
napi_create_double:return jsNumber(purifyNaN(cell.value.number));Purifying at the sink covers every
DataCellTag::Doubleproducer (Postgres and MySQL,float8andfloat4). Non-NaN doubles are untouched; any NaN bit pattern collapses to the canonical NaN.Verification
New test
test/js/sql/postgres-binary-float-nan-box.test.tsstands up a mock TCP server speaking the real Postgres v3 wire protocol and returns a binaryfloat8column with forged bit patterns that previously decoded asboolean true,undefined, and an int32. With the fix each comes back as the numberNaN; an ordinary double (3.5) round-trips unchanged.USE_SYSTEM_BUN=1): the forged columns returnboolean/undefined, assertions fail.bun bd test): 4 pass.