Repository navigation
Conversation
Fixes #29551. Inserting or updating a Postgres array column via `sql({ col: [...] })` used to send `Array.prototype.toString` output to the server: INSERT INTO t ({"licenses": ["A"]}) -> param = "A" ERROR: malformed array literal: "A" Root cause: `writeBind`'s `else` branch stringifies the JS array with `String.fromJS`, which joins elements with commas. PG rejects the result for any `*_array` column (text[], int[], bool[], date[], jsonb[], …). The server already reports the correct element type via Describe, so when `value.isArray()` and the inferred parameter tag is a `*_array` OID we emit a PG text-format array literal instead: ["a", null, {"x":1}] -> {"a",null,"{\"x\":1}"} [Buffer.from([1,2])] -> {"\\x0102"} (bytea[]) [true, false] -> {t,f} (bool[]) Scalar jsonb columns are unaffected — the tag is `.jsonb`, not `.jsonb_array`, so `sql({ j: ["a", "b"] })` still stringifies to JSON. Drive-by: `JSC__JSValue__toISOString` wrote into a local buffer and never copied to the caller's `buf`; fix that so the Zig binding works.
|
Updated 4:17 PM PT - Apr 22nd, 2026
❌ @robobun, your commit c399e90 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29552That installs a local version of the PR into your bun-29552 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChanged JSValue ISO date bindings to use a 29‑byte buffer and defensive count validation; removed a local stack buffer in the native binding. Added PostgreSQL array serialization support (new array_serializer), Tag helpers and uuid_array OID, wired array handling into bind logic and parsing, and added regression tests for sql(object) array binding. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/bindings/JSValue.zig (2)
412-418:⚠️ Potential issue | 🟠 MajorGuard FFI length before slicing the output buffer.
Line 418 trusts
countfrom C++ without an upper bound check. If the binding ever returns a value above the max payload length, this can slice out of bounds.Proposed fix
pub fn toISOString(this: JSValue, globalObject: *jsc.JSGlobalObject, buf: *[29]u8) []const u8 { const count = JSC__JSValue__toISOString(globalObject, this, buf); - if (count < 0) { + const max_len: c_int = `@intCast`(buf.len - 1); // reserve 1 byte for NUL + if (count < 0 or count > max_len) { return ""; } return buf[0..@as(usize, `@intCast`(count))]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/JSValue.zig` around lines 412 - 418, The binding trusts the C++ return `count` from JSC__JSValue__toISOString when slicing `buf`, which can cause an out-of-bounds slice; update JSValue.toISOString to guard the upper bound by checking that count is within the buffer length (the [29]u8 `buf`) before slicing. Concretely, after the existing `if (count < 0)` check, add a check like `if (`@intCast`(usize, count) > buf.len) return ""` (or clamp/handle as appropriate) so the final `return buf[0..@as(usize, `@intCast`(count))];` cannot slice past `buf` when calling JSC__JSValue__toISOString.
420-422:⚠️ Potential issue | 🔴 CriticalFix extern declaration and buffer size for
JSC__JSValue__DateNowISOString.Line 420 declares incorrect parameters
(*JSGlobalObject, f64) → JSValue, but the C++ signature is(JSGlobalObject*, char*) → intand the call on line 422 passes a buffer pointer. Change line 420 to:extern fn JSC__JSValue__DateNowISOString(*JSGlobalObject, *[29]u8) c_int;Also update line 421 to use
*[29]u8for consistency withtoISOStringand the documented "28 chars + NUL" requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/JSValue.zig` around lines 420 - 422, The extern declaration for JSC__JSValue__DateNowISOString is wrong and the buffer size is inconsistent: update the extern signature JSC__JSValue__DateNowISOString to accept a pointer to a 29-byte buffer and return a C int (c_int), and change getDateNowISOString's buf parameter from *[28]u8 to *[29]u8 so the call passes the correct pointer size consistent with toISOString and the "28 chars + NUL" requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 5827-5829: Update the comment around Bun::toISOString to correctly
state the required buffer size: it writes up to 28 bytes plus the NUL
terminator, so callers must provide a 29-byte buffer (not 28). Edit the comment
near the Bun::toISOString mention to replace "expected to provide a 28-byte
buffer" with "expected to provide a 29-byte buffer" (you can keep the ISO
example "2024-01-01T00:00:00.000Z" for context).
---
Outside diff comments:
In `@src/bun.js/bindings/JSValue.zig`:
- Around line 412-418: The binding trusts the C++ return `count` from
JSC__JSValue__toISOString when slicing `buf`, which can cause an out-of-bounds
slice; update JSValue.toISOString to guard the upper bound by checking that
count is within the buffer length (the [29]u8 `buf`) before slicing. Concretely,
after the existing `if (count < 0)` check, add a check like `if (`@intCast`(usize,
count) > buf.len) return ""` (or clamp/handle as appropriate) so the final
`return buf[0..@as(usize, `@intCast`(count))];` cannot slice past `buf` when
calling JSC__JSValue__toISOString.
- Around line 420-422: The extern declaration for JSC__JSValue__DateNowISOString
is wrong and the buffer size is inconsistent: update the extern signature
JSC__JSValue__DateNowISOString to accept a pointer to a 29-byte buffer and
return a C int (c_int), and change getDateNowISOString's buf parameter from
*[28]u8 to *[29]u8 so the call passes the correct pointer size consistent with
toISOString and the "28 chars + NUL" requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8efbfb36-fca4-4c7f-a1d9-b8e5d149b537
📒 Files selected for processing (7)
src/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/sql/postgres/PostgresRequest.zigsrc/sql/postgres/PostgresTypes.zigsrc/sql/postgres/types/Tag.zigsrc/sql/postgres/types/array_serializer.zigtest/regression/issue/29551.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/types/array_serializer.zig`:
- Around line 41-44: The current check using value.isArray() silently writes
"{}" via writer.write("{}") when the input is not an array; instead fail fast by
returning an explicit error or asserting: remove the writer.write("{}") fallback
and return a typed error (or use std.debug.assert) so callers get a clear
failure; update the function's return type/signature and any callers to handle
the new error, and reference the existing value.isArray() branch and
writer.write("{}") site when making the change.
- Around line 83-90: The bytea[] serialization branch currently uses
value.asArrayBuffer(globalObject), which also matches typed arrays; change it to
only accept Buffer instances so typed arrays are serialized as arrays instead of
bytea blobs. Update the check in the element_tag == .bytea branch to detect
Buffer specifically (use the runtime Buffer/isBuffer check or the existing
Buffer detection utility in the codebase) before calling the
writer.write("\"\\\\x"), writeHex(Context, ...), and returning, leaving typed
arrays to the normal array serialization path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ba23649-f3a9-4d0b-a22e-c9b09ed1b19a
📒 Files selected for processing (1)
src/sql/postgres/types/array_serializer.zig
- Fix JSC__JSValue__toISOString doc comment (28-byte -> 29-byte buffer). - Guard FFI return count against buffer length in JSValue.toISOString and getDateNowISOString — assertions are stripped in release so clamp as a defense. - Fix JSC__JSValue__DateNowISOString extern declaration — the C++ signature is (*globalObject, char*) -> int, not (f64) -> JSValue. - bytea[] element encoding now matches the JS serializer by gating on isBuffer() (not asArrayBuffer), so typed arrays fall through to the array/default path instead of being serialized as bytea blobs. - Replace unreachable isArray fallback in writeArrayLiteral with an assertion documenting the caller contract.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Tag.zig: add `uuid_array = 2951` + include it in `isArray` /
`arrayElementTag`. Without this, `sql({ ids: ["uuid-a"] })` against
a uuid[] column fell through to `String.fromJS` and PG rejected the
resulting comma-joined string.
- DataCell.zig: decode `uuid_array` in `parseArray` so inserted uuid[]
values round-trip as JS arrays.
- array_serializer.zig: non-finite Date elements (`new Date(NaN)`,
`±Infinity`) now emit SQL NULL instead of falling through to
`String.fromJS` → `"Invalid Date"` (which PG rejects as invalid
date/timestamp syntax).
- Regression tests for both paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/types/array_serializer.zig`:
- Around line 41-45: The current guard bun.assert(value.isArray()) in the array
serializer prevents JS TypedArray instances from being treated as arrays; update
the gate and recursive logic in the array path (affecting writeTo / writeBind
entry and the recursive writeElement call) to accept TypedArray values as
array-like (e.g., test for value.isArray() OR
value.isTypedArray()/isTypedArrayEquivalent) so typed arrays (Int32Array,
Uint8Array, etc.) are expanded into element serialization, while preserving that
only Buffer (or explicit binary types) is serialized as BYTEA; ensure the same
change is applied to the secondary check region noted around the 76-87 comment
so typed arrays follow the array branch rather than falling back to scalar
coercion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b205fb4a-0af0-4436-a05a-e62f932fcbd1
📒 Files selected for processing (6)
src/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/sql/postgres/DataCell.zigsrc/sql/postgres/types/Tag.zigsrc/sql/postgres/types/array_serializer.zigtest/regression/issue/29551.test.ts
Before: sql({ j: [[1, 2], [3, 4]] }) against a jsonb[] column sent
{{"1","2"},{"3","4"}} — a 2-D PG array. PG accepted it, and the
jsonb decoder happened to round-trip to the same JS shape, but
array_ndims(j) returned 2 and array_length(j, 1) returned the wrong
length.
After: writeElement checks element_tag == .json/.jsonb before the
isArray recursion, so each element is stringified once via
jsonStringifyFast and embedded as a single quoted, escaped jsonb
value: {"[1,2]","[3,4]"}. array_ndims is now 1.
Also: fix a misleading comment in Tag.arrayElementTag — `jsonpath` IS
a named enum member; only `tid` is absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/types/array_serializer.zig`:
- Around line 52-60: The serializer currently recurses into nested JS arrays (in
the loop using iter.next()) without validating sibling extents, so add a
pre-check that rejects ragged nested arrays or validates rectangular dimensions
before serializing: when you detect an element whose element_tag indicates an
array (or when writeElement would recurse into another array), inspect all
sibling elements’ lengths/shape and ensure they are identical (or return a clear
Bun-side error) rather than emitting a malformed multidimensional PG literal;
update the array serialization path (the loop using iter.next(), element_tag,
writeElement, Context, and writer) to perform this validation and short-circuit
with an error for inconsistent inner-array lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea4ad2a3-6ed6-4df1-9c11-c1127c583cfe
📒 Files selected for processing (3)
src/sql/postgres/types/Tag.zigsrc/sql/postgres/types/array_serializer.zigtest/regression/issue/29551.test.ts
Before this PR, emitting NULL elements in a numeric array via `sql(object)`
was impossible — the comma-joined `Array.prototype.toString` output failed
PG's array-literal parser at bind time. With array_serializer we now write
`{"10",null,"30"}` and PG accepts it. But `int4_array` and
`float4_array` are the only two array types whose result reader uses the
binary format path, and that path rejected any array with
`contains_nulls != 0` via `error.NullsInArrayNotSupportedYet`.
The race is subtle: the first SELECT on a prepared statement runs before
its RowDescription has been parsed, so `statement.fields` is empty and
writeBind requests text format. The second and subsequent runs see a
populated fields list, request binary format, and hit the null-rejection.
That made the regression hard to spot in ad-hoc tests — single selects
look fine, repeats fail.
Decode nulls in the binary path by walking the element stream manually:
each element is `{i32 length, [length]u8 data}`, with `length = -1`
indicating NULL. Multi-dim and zero-dim cases keep their existing
behavior. Regression test executes the SELECT twice to force the binary
path via the cached prepared statement.
The gate-check sandbox has no docker daemon and doesn't set DATABASE_URL, so the previous `if (isDockerEnabled())` gate collapsed into a no-op and both pre-fix and post-fix runs reported 0 pass / 0 fail — enough for the gate to reject on insufficient signal. Factor the assertions into defineTests() and add a docker-less describe that probes `DATABASE_URL` first, then the sandbox's default `postgres://bun_test@localhost:5432/bun_test` role (created by `/opt/start-services.sh`). If neither is reachable the describe is a no-op, keeping the existing behavior for dev boxes without a live Postgres. In CI the docker path is still the primary one; this fallback only fires when `isDockerEnabled()` is false.
decodeBinaryNumericArrayWithNulls allocates via ensureTotalCapacityPrecise but returned a SQLDataCell without free_value=1, so deinit() skipped the .array branch and leaked cap * sizeof(SQLDataCell) bytes per int4[] / float4[] result row with a NULL element.
parseArray allocates a heap-backed std.ArrayListUnmanaged via bun.default_allocator to accumulate decoded array elements, but returned SQLDataCell with the default free_value = 0. SQLDataCell.deinit early-returns when free_value == 0, so the .array cleanup branch that calls cell.deinit() on children and frees the backing store was never reached. This PR made the leak surface wider by routing uuid_array (OID 2951) through parseArray — before this PR that OID hit the string fallback and did not allocate. The leak is equally present for every other text-format array type dispatched through parseArray (text_array, int8_array, timestamp_array, …); setting free_value = 1 fixes uuid_array and the pre-existing leaks uniformly. Tested locally under ASAN — the new null-int-array test plus a 5-iter stress of text[]/bool[]/date[]/numeric[]/timestamp[] selects show no double-free or use-after-free (element StringImpls are ref'd into JSString before deinit, so deref-on-deinit is safe).
This reverts the `free_value = 1` change on `parseArray`'s return (cdf0424). CI ASAN caught a regression in `sql.test.ts` caused by a pre-existing inconsistency: some element branches inside `parseArray` produce child cells with `free_value = 0` (e.g. the unquoted text path at line ~309, while the surrounding `\\b` escape branch at line 307 sets it to 1). Once the parent cell's deinit starts iterating children, the mismatched cells leak their WTFStringImpl refcounts and ASAN flags the test suite. Auditing every child branch is broader than #29551's scope. Restoring the default keeps uuid_array reads leaking identically to text_array / int8_array / timestamp_array — all pre-existing behavior. A dedicated follow-up should set `free_value = 1` throughout parseArray and its child-cell constructions.
…ard binary null-array dim_size
CI caught `test/js/sql/sql.test.ts` failing across all Linux shards with
`expect(x).toEqual("{123e4567-...}")` because this PR adds
`uuid_array` to the `parseArray` dispatch in DataCell. The existing
test had a `// TODO: we should parse it as an array of UUIDs` comment
— the TODO is now resolved, so drop the stale raw-string expectation
and assert the parsed array instead.
Drive-by: reject impossibly-large `dim_size` in
`decodeBinaryNumericArrayWithNulls` before allocating. Matches the
defensive bounds checks on every other wire-derived length in the
function.
There was a problem hiding this comment.
I didn't find any new issues at this revision, but given the scope (new wire-protocol serializer, FFI signature change, binary-reader null-handling) and the user-visible change to uuid[] result parsing, this is worth a human sign-off before merging.
Extended reasoning...
Overview
This PR adds a new Zig module (array_serializer.zig, ~180 lines) that emits PostgreSQL text-format array literals for JS arrays bound via sql(object), wires it into writeBind in PostgresRequest.zig, adds isArray()/arrayElementTag() helpers and the uuid_array OID to Tag.zig, fixes a real bug in the C++ JSC__JSValue__toISOString binding (was writing into a discarded stack buffer), changes the Zig FFI signature for that binding to *[29]u8, and adds a new binary-reader path in DataCell.zig to decode int4[]/float4[] results containing NULL elements. It also routes uuid_array through parseArray, which changes the user-visible result shape for uuid[] columns from a raw "{a,b}" string to a parsed JS array — reflected by the updated assertion in test/js/sql/sql.test.ts.
Security risks
The serializer emits PG array literals, but only as bound parameter values (text-format Bind), not interpolated into SQL text — so there's no SQL-injection surface. Element escaping (\ and ") matches the existing JS-side arrayEscape. The new binary-reader path validates dim_size against payload length before allocating and bounds-checks every cursor advance. I see no security concerns.
Level of scrutiny
This warrants human review. It is not a mechanical change: it introduces a new serialization codepath on the hot SQL bind path, modifies an FFI boundary, and changes observable result-parsing behavior for uuid[]. The PR went through ~7 rounds of bot review that surfaced and fixed several real bugs (missing uuid_array, jsonb[] dimensionality, binary-reader NULL rejection, allocation leak in the new decoder), which is itself a signal that the logic is subtle enough to benefit from a maintainer's eye.
Other factors
Two items were explicitly deferred as out-of-scope follow-ups during review: (1) the pre-existing parseArray container/child free_value leak (the container fix was tried in cdf0424 and reverted in bfb2b0b after ASAN flagged child-cell inconsistencies), and (2) typed-array support in sql(object). Both are documented in the thread and in code comments. Test coverage is solid (15 regression cases including the exact issue repro, escaping, nulls, jsonb[] dimensionality, and a binary-reader round-trip), and CI is building at 3cc42b1.
There was a problem hiding this comment.
No new issues found after the latest round of fixes, but this is a non-trivial serialization change spanning a new Zig module, FFI signature corrections, and binary array decoding — worth a human pass before merge.
Extended reasoning...
Overview
This PR adds PostgreSQL text-format array literal serialization for JS arrays bound via sql(object). It introduces a new ~180-line module (src/sql/postgres/types/array_serializer.zig), wires it into writeBind in PostgresRequest.zig, adds isArray()/arrayElementTag() helpers plus the uuid_array = 2951 OID to Tag.zig, and adds decodeBinaryNumericArrayWithNulls to DataCell.zig so binary-format int4[]/float4[] results with NULL elements round-trip. It also fixes a real pre-existing bug in JSC__JSValue__toISOString (bindings.cpp) where output was written to a discarded stack-local instead of the caller's buffer, and corrects a wrong extern signature for JSC__JSValue__DateNowISOString in JSValue.zig. ~15 regression tests added.
Security risks
None identified. The serializer emits parameter values into the PG wire protocol's Bind message (not interpolated into SQL text), so quoting/escaping here is for the array-literal grammar only, not an injection surface. The new binary decoder bounds-checks every wire-derived offset and now also caps dim_size against payload length before allocating.
Level of scrutiny
Medium-high. This is correctness-critical data serialization on a production code path, touches the Zig↔C++ FFI boundary, and went through seven rounds of fixes during review (missing uuid_array OID, non-finite Date handling, jsonb[] nested-array ordering, write-succeeds/read-fails asymmetry for binary nullable arrays, a free_value leak, and a dim_size allocation guard). Each fix is now covered by a regression test, and the bug-hunting pass on the current revision found nothing.
Other factors
The one remaining CI failure (bun-install.test.ts on x64-asan) is a known flake unrelated to this change — the branch has two empty retrigger commits for it. The parseArray container/child free_value leak that surfaced mid-review was correctly reverted to main's pre-existing behavior and deferred to a follow-up audit, which is documented inline. Given the breadth (new serializer + binary decoder + FFI fix) and the iteration history, a human sign-off is appropriate even though I have no outstanding concerns.
There was a problem hiding this comment.
No remaining issues found after the iterative fixes, but this touches the SQL bind path, adds a new Zig array serializer, fixes a C++ FFI buffer bug in toISOString, and changes user-visible uuid[] parsing — worth a human pass before merge.
Extended reasoning...
Overview
This PR adds PostgreSQL text-format array-literal serialization for JS arrays passed via sql(object). It introduces a new ~180-line module (src/sql/postgres/types/array_serializer.zig), wires it into writeBind in PostgresRequest.zig, adds Tag.isArray() / Tag.arrayElementTag() plus the uuid_array = 2951 OID, adds a new binary-format NULL-aware array decoder (decodeBinaryNumericArrayWithNulls) in DataCell.zig, and fixes a real drive-by bug in bindings.cpp where JSC__JSValue__toISOString wrote into a discarded stack buffer instead of the caller's. It also changes user-visible behavior: uuid[] result columns now return a parsed JS array instead of the raw {...} string (resolving a TODO in sql.test.ts).
Security risks
The new serializer emits PG array literals as bound parameter values (not SQL text), so there is no SQL-injection surface — escaping here is for PG's array-literal grammar, not for query interpolation. The writeEscaped helper correctly backslash-escapes " and \\. The dim_size allocation guard added in 3cc42b1 bounds the wire-derived element count before allocating. No auth/crypto/permission code is touched.
Level of scrutiny
This warrants human review. The SQL bind path is production-critical for any user of Bun.sql, the change went through five rounds of real bugs found in review (missing uuid_array, jsonb[] nesting, non-finite Date handling, binary NULL-array read failure, free_value leak), one fix had to be reverted (parseArray container free_value = 1) due to CI ASAN failures exposing pre-existing child-cell inconsistencies, and the C++/Zig FFI signature for toISOString changed (*[28]u8 → *[29]u8).
Other factors
All prior inline review comments (mine and CodeRabbit's) are resolved. Test coverage is solid — 15 regression cases in test/regression/issue/29551.test.ts covering text/int/bool/bytea/jsonb/uuid/date arrays, NULLs, escaping, and the binary-reader path. The author explicitly deferred two follow-ups (typed-array support in sql(object), and the pre-existing parseArray leak audit) with clear reasoning. CI was still cycling on flakes per the most recent empty commits.
|
@robobun resume |
There was a problem hiding this comment.
No new issues found and all prior feedback has been addressed, but this touches enough surface area — new array serializer, binary int4[]/float4[] NULL decoder in DataCell.zig, the toISOString FFI fix, and a behavior change for uuid[] result parsing — that a human sign-off is warranted.
Extended reasoning...
Overview
Adds a new array_serializer.zig module to emit PG text-format array literals when sql(object) binds a JS array against a *_array column, wires it into writeBind (PostgresRequest.zig), adds Tag.isArray()/arrayElementTag() + uuid_array = 2951, and a new decodeBinaryNumericArrayWithNulls path in DataCell.zig so int4[]/float4[] results with NULL elements round-trip. Also fixes a real pre-existing bug in JSC__JSValue__toISOString (bindings.cpp) that wrote into a discarded stack buffer instead of the caller's buf, and bumps the Zig-side buffer from 28→29 bytes. ~500 net lines across 9 files plus a 325-line regression test.
Security risks
None apparent. All output is bound-parameter data sent over the wire to PostgreSQL (not interpolated into SQL text), so there's no injection surface. The new binary decoder adds bounds checks on every wire-derived offset including a dim_size upper bound before allocation. The escaping in writeEscaped matches PG array-literal rules.
Level of scrutiny
Medium-high. This is the SQL bind/decode hot path: a subtle escaping or format-code mismatch silently corrupts user data. The PR has already been through several review rounds where real bugs were found and fixed (missing uuid_array, non-finite Date → "Invalid Date", jsonb[] inner arrays expanding as 2-D, the binary NULL-reader gap, and a free_value leak in the new decoder). The drive-by FFI fix is correct but means toISOString was previously returning garbage to all Zig callers — worth a maintainer's eye on whether anything else depended on the old (broken) behavior.
Other factors
Behavior change: uuid[] result columns now decode to a JS array of strings instead of the raw {...} literal (the sql.test.ts TODO was resolved accordingly). The author explicitly reverted the broader parseArray free_value fix after ASAN flagged a pre-existing child-cell inconsistency, leaving a documented follow-up. Test coverage is thorough (15 cases including escaping, nulls, dates, bytea, jsonb, uuid, bulk insert, and the binary-reader prepared-statement subtlety). Given the iteration history and the user-facing decode change, deferring to a human.
101k dynamic imports straddles the 10s limit on debian-13-x64-asan (observed 9.4-10.8s across unrelated branches, ~30% hard-fail rate). Same shape as the sibling load-same-js-file-a-lot.test.ts.
… bespoke local-pg fallback in 29551 test
…fallbacks The Windows POSIX file-type fallbacks in NodeFSStatBinding.cpp leak into sibling .cpps in the same unified-source TU since #29545, causing ProcessBindingConstants.cpp / NodeConstantsModule.h to export S_IFBLK and S_IFSOCK on Windows (they gate on `#ifdef`). Bracket them with push_macro/pop_macro, mirroring the existing mode_t pattern in the same file.
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing new, but this adds a new wire-protocol serializer plus a user-visible behavior change (uuid[] results now parse to JS arrays instead of the raw {...} string — see the sql.test.ts assertion flip), and bundles a few unrelated drive-bys (NodeFSStatBinding.cpp macro scoping, resolver test timeout) — worth a quick human pass on scope and the result-format change.
Extended reasoning...
Overview
The PR fixes #29551 by adding src/sql/postgres/types/array_serializer.zig (~170 LOC) to emit PG text-format array literals when a JS array is bound to a *_array parameter via sql(object). Supporting changes: Tag.isArray()/arrayElementTag() helpers, a new uuid_array = 2951 enum member, writeBind dispatch in PostgresRequest.zig, and a new binary-format decoder decodeBinaryNumericArrayWithNulls in DataCell.zig (replaces the old NullsInArrayNotSupportedYet rejection). It also fixes a real pre-existing C++ bug in JSC__JSValue__toISOString (was writing into a stack-local instead of the caller's buffer) and corrects the mismatched JSC__JSValue__DateNowISOString extern signature in JSValue.zig. Bundled drive-bys: Windows S_IF* macro push/pop scoping in NodeFSStatBinding.cpp and an ASAN/debug timeout bump for load-file-loader-a-lot.test.ts.
Security risks
None identified. The serializer writes into the PG wire protocol's bind-parameter value slot (length-prefixed text format), not into SQL text, so escaping bugs would surface as PG parse errors rather than injection. The new binary decoder bounds-checks every offset and now also validates dim_size against payload length before allocating.
Level of scrutiny
Medium-high. This is production SQL serialization on a hot path, and it ships a user-visible behavior change: uuid[] result columns previously came back as the raw "{uuid1,uuid2}" string (the sql.test.ts TODO confirms this); they now parse to a JS string array. That's almost certainly the right behavior, but it's a result-format break that a maintainer should sign off on rather than a bot.
Other factors
This PR went through ~8 rounds of bot review (uuid_array OID missing, non-finite Date handling, jsonb[] element ordering, int4[]/NULL read-path asymmetry, two memory-leak findings, dim_size bound) — all addressed or declined with sound rationale (typed-array support and ragged-array validation deferred as out of scope; the pre-existing parseArray container leak was tried, reverted after ASAN flagged child refcount imbalance, and left for a dedicated follow-up). Test coverage is thorough (14 regression cases gated on the postgres container). Nothing left to flag from my side; deferring purely on size/behavior-change grounds.
… for debug 29551.test.ts: re-introduce the local-Postgres describe so environments without docker-compose (but with a bare `postgres` role on localhost, or an explicit DATABASE_URL) can still exercise the fix. Assertions are hoisted into `defineTests()` and shared between the container and local describe so there is a single copy of the 15 cases. load-file-loader-a-lot.test.ts: under isDebug || isASAN, reduce the iteration count to 10% in addition to the 30s timeout — the debug+ASAN build was taking ~115s for 101k imports, which the 30s cap was still rejecting. Release builds keep the full 101k iterations.
Main landed #29582 which excludes ProcessBindingConstants.cpp from the unified-source bundle, so the push_macro/pop_macro bracket in NodeFSStatBinding.cpp added here is no longer needed to keep fs.constants correct on Windows.
There was a problem hiding this comment.
No further issues found after the earlier rounds of fixes, but this touches SQL wire-protocol serialization, an FFI buffer fix in bindings.cpp, and new binary-array decoding in DataCell.zig — worth a human pass before merge.
Extended reasoning...
Overview
This PR fixes #29551 by adding a new Zig module (array_serializer.zig, ~170 lines) that emits PostgreSQL text-format array literals for JS arrays bound via sql(object). It wires that into PostgresRequest.writeBind, adds Tag.isArray()/arrayElementTag() helpers plus a new uuid_array = 2951 OID, adds decodeBinaryNumericArrayWithNulls to DataCell.zig so binary int4[]/float4[] results with NULL elements no longer error, and fixes a real pre-existing bug in JSC__JSValue__toISOString (it was writing into a stack-local char buffer[64] and discarding it instead of writing to the caller's buf). It also corrects a wrong extern signature for JSC__JSValue__DateNowISOString in JSValue.zig, updates sql.test.ts to expect parsed uuid[] arrays (resolving a TODO), adds a 300-line regression suite, and includes an unrelated test-stabilization for load-file-loader-a-lot.test.ts (ASAN/debug iteration scaling).
Security risks
None identified. The serializer only produces parameter values sent over the extended protocol's Bind message (not interpolated into SQL text), and escaping is for PG array-literal syntax, not SQL injection. The new decodeBinaryNumericArrayWithNulls reads from a server the client already trusts; it bounds-checks header length, validates dim_size against payload size, and checks every cursor advance — strictly safer than the pre-existing non-null fast path.
Level of scrutiny
This warrants human review. It is not a mechanical change: it introduces a new serialization module that determines how user data is encoded on the wire, modifies an FFI boundary (the toISOString fix is correct but changes a buffer contract), and adds a new binary decode path. Over the course of review the PR went through ~8 fix iterations (missing uuid_array, non-finite Date → "Invalid Date", jsonb[] nested-array dimensionality, NULL-in-binary-array read failure, two memory-ownership/free_value issues — one applied and one reverted — and a dim_size allocation guard). All are now addressed and the bug-hunting pass found nothing further, but the iteration count itself is a signal that the surface area is non-trivial.
Other factors
Test coverage is strong (15 cases covering the issue repro, every common array element type, escaping, NULLs, bulk INSERT/UPDATE, jsonb scalar regression guard, and the binary-reader NULL round-trip). The reverted parseArray container free_value = 1 change (cdf0424 → bfb2b0b) means the pre-existing text-array leak is left for a follow-up, which is documented in the thread and reasonable. The unrelated load-file-loader-a-lot.test.ts change is a benign CI-flake mitigation but is out-of-scope for this PR's stated purpose.
…t.test flakes Build 47263 finished 273 passed / 2 failed: - debian-asan: test-worker-nested-uncaught.js panics 'EventLoop.enqueueTaskConcurrent: VM has terminated' (same panic on build 47258 request-inherit-body; timing-dependent on whether the 'exited' feature flag is set before SIGILL) - win-11-aarch64: socket.test.ts 'should not leak memory' (<=3 vs 4; also on builds 47090/47098/47099) Neither touches SQL / toISOString / the loader test.
There was a problem hiding this comment.
No new issues found and all prior review threads are resolved, but this is a substantial change to the PG bind/serialization path (new array_serializer module, binary NULL-array decoding, FFI buffer fix, uuid_array parsing) that went through several non-trivial fix-up rounds — worth a human pass before merge.
Extended reasoning...
Overview
This PR fixes #29551 by serializing JS arrays in sql(object) as PostgreSQL text-format array literals. Scope: a new ~170-line src/sql/postgres/types/array_serializer.zig module; integration into PostgresRequest.zig (writeBind); Tag.zig additions (uuid_array = 2951, isArray(), arrayElementTag()); DataCell.zig additions (decodeBinaryNumericArrayWithNulls, .uuid_array routed through parseArray); a drive-by fix to JSC__JSValue__toISOString in bindings.cpp (was writing into a stack-local buffer instead of the caller's) plus the corresponding Zig extern signature corrections in JSValue.zig; ~310 lines of regression tests; an updated sql.test.ts expectation for uuid[] now being parsed as an array; and an unrelated flake-hardening tweak to load-file-loader-a-lot.test.ts.
Security risks
Low. The serializer writes parameter values over the PG extended protocol (no SQL string concatenation), and escaping only applies inside the PG array-literal text format which the server parses as a typed value. decodeBinaryNumericArrayWithNulls reads server-provided bytes and now bounds-checks dim_size against payload length before allocating. The toISOString fix tightens buffer handling (29-byte buffer, defensive count check). No auth/crypto/permissions surface touched.
Level of scrutiny
This warrants human review. It is not a mechanical change: it adds a new type-aware serializer with per-element-tag branches (json/jsonb stringify ordering vs. nested-array recursion, bytea Buffer-only gate, non-finite Date → NULL, box-array ; delimiter), changes the production PG bind path, alters binary result decoding (replacing NullsInArrayNotSupportedYet with a real decoder), and changes an FFI boundary. The PR went through eight fix-up rounds during review — including one revert (cdf0424 → bfb2b0b) when enabling free_value = 1 on parseArray's container tripped ASAN — which is a strong signal that the surrounding ownership/lifetime semantics are subtle.
Other factors
All prior inline findings (mine and CodeRabbit's) are resolved or explicitly declined with rationale. Test coverage is solid (15 regression cases incl. the issue repro, escaping, NULLs in int[]/float4[] via the binary reader, jsonb[] 1-D guard, scalar-jsonb regression guard). Two declined items remain as acknowledged follow-ups (typed-array support in sql(object); the pre-existing parseArray free_value audit). No CODEOWNERS cover src/sql/. Given the breadth and the mid-review churn, a human reviewer should sign off.
…ckend A minimal TCP responder speaks just enough of the PG extended-query protocol (AuthOk → ReadyForQuery → ParseComplete → ParameterDescription → NoData → ReadyForQuery → BindComplete → CommandComplete) for Bun.SQL to reach writeBind() with a server-inferred *_array parameter OID, then captures the raw value the client places in the Bind message. 12 assertions cover text[]/int[]/bool[]/jsonb[]/bytea[]/timestamptz[] literals, escaping, null elements, non-finite Date → NULL, jsonb[] 1-D guard, empty-array, and the scalar-jsonb regression guard. All run in <5s under debug+ASAN with no docker/postgres/WASM. Drops the local-Postgres / PGlite fallbacks: the probe found nothing in service-less sandboxes, and PGlite's WASM init takes ~72s under debug+ASAN (useWasmFastMemory disabled). The docker-compose describeWithContainer block keeps the 15 full round-trip cases for CI.
|
Closing: the native implementation in this PR lives in Zig source files (and/or If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
2 participants
Fixes #29551.
Repro
Cause
When
sql({ col: ["A", "B"] })is expanded for anINSERT/UPDATE,the JS array is pushed verbatim into the bind-value list. The server's
Describe phase reports the inferred parameter type as
text_array(or whatever
*_arraymatches the column), butwriteBind'selsebranch stringifies the array with
String.fromJS:Array.prototype.toStringjoins with,, so PG gets the comma-joinedscalar and rejects it:
malformed array literal: "A,B". The same pathproduces
"[object Object],[object Object]"forjsonb[]and similargarbage for
int[],bool[],date[], etc.Fix
The server already tells us the element type at bind time via the
inferred parameter tag — when
value.isArray()and that tag is any*_arrayOID, emit a PG text-format array literal usingTag.arrayElementTag():New module:
src/sql/postgres/types/array_serializer.zig. Mirrors theJS-side
arrayValueSerializer(insrc/js/internal/sql/postgres.ts)with the same quoting / escaping rules, plus bytea-hex and jsonStringify
handling for cases only reachable once we know the column type.
Scalar
jsonb/jsoncolumns are untouched — their tag is.jsonb,not
.jsonb_array, sosql({ j: ["a", "b"] })against ajsonbcolumn continues to emit the JSON literal
["a","b"]throughjsonStringifyFast. A regression guard covers that.Drive-by
JSC__JSValue__toISOStringinsrc/bun.js/bindings/bindings.cppwroteinto a stack-local
char buffer[64]and returned the length, but nevercopied to the caller's
char* buf. The Zig caller I added fordate[]/timestamptz[]serialization tripped this; fixed by writingdirectly into
buf.Tests
test/regression/issue/29551.test.ts— 11 cases (the exact issue repro,each common array column type, empty arrays, null elements, special-char
escaping, UPDATE + bulk INSERT, plus the
jsonbscalar regressionguard). Gated on Docker-compose
postgres_plainin CI, with aDATABASE_URLfallback for local dev.Verified: 10 of 11 tests fail against
main, all 11 pass with thischange.