postgres: validate binary datum length and range before decoding - #33576
postgres: validate binary datum length and range before decoding#33576robobun wants to merge 7 commits into
Conversation
A RowDescription can declare format=1 (binary) for a column, after which
the server (or a compromised pooler/proxy) authors the datum bytes. Several
binary decode arms in DataCell trusted those bytes and silently turned a
wire-level violation into a plausible JS value instead of rejecting it:
- float8/float4/timestamp/timestamptz with a wrong-width datum fell through
to the text parser, yielding NaN or Invalid Date
- bool accepted a 0-byte datum (false) or a value outside {0, 1}
- binary int4[]/float4[] ignored each element's length prefix, so a prefix
other than the element size was decoded as if it matched
- time/timetz fed an out-of-range or negative microsecond count straight to
the C formatter, producing garbage strings
- numeric accepted trailing bytes after an ndigits=0 header and a negative
(high-bit) dscale
- a binary format code on a type with no binary decoder (e.g. uuid) was
silently reinterpreted as text, exposing the raw bytes as a JS string
Each arm now validates the datum against the declared type and errors with
the matching ERR_POSTGRES_* code, the way the int4 arm already did.
|
Warning Review limit reached
Next review available in: 3 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 (3)
WalkthroughThis PR adds stricter binary-format validation to PostgreSQL datum decoding in DataCell.rs, including exact length checks for FLOAT8/FLOAT4/BOOL/TIMESTAMP, microsecond range checks for TIME/TIMETZ, typed-array element length validation, numeric header validation, and format-code enforcement in put_impl. A new test suite validates malformed and well-formed binary payloads via a mock server. ChangesPostgres binary datum validation hardening
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:05 AM PT - Jul 7th, 2026
❌ @robobun, your commit 93f2dfa has some failures in 🧪 To try this PR locally: bunx bun-pr 33576That installs a local version of the PR into your bun-33576 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sql_jsc/postgres/DataCell.rs`:
- Around line 1304-1309: The binary-format guard in DataCell::from_pg_result
only checks FieldDescription::binary, so prepared-statement results can still
fall through because self.binary is the flag that enables binary decoding there.
Update the unsupported-binary check in the DataCell decoding path to reject when
either the row description or the active result path indicates binary, and keep
using tag.is_binary_format_supported() as the decoder capability check.
In `@test/js/sql/postgres-binary-datum-validation.test.ts`:
- Around line 165-198: The binary datum validation suite runs I/O-bound cases
sequentially even though each test creates its own server and Postgres
connection via runMockQuery, so the malformed table-driven cases and the
standalone well-formed parsing checks should be made concurrent. Update the
existing test.each and test blocks in postgres-binary-datum-validation.test.ts
to use test.concurrent.each and test.concurrent for the malformed set and the
individual bool/float8/time/numeric tests, keeping the same assertions and
helpers but enabling parallel execution safely.
🪄 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: 39af7989-d433-4898-9d34-45432915d13c
📒 Files selected for processing (2)
src/sql_jsc/postgres/DataCell.rstest/js/sql/postgres-binary-datum-validation.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The pre-existing is_empty() -> null early-return ran before the binary length check, so a 0-byte binary timestamp/timestamptz/time/timetz datum still surfaced as null instead of ERR_POSTGRES_INVALID_BINARY_DATA. Move the is_empty() -> null shortcut into the text branch of both arms so the binary width check runs first. Also add a float4 2-byte malformed case and timestamp/time 0-byte cases to the fault-injection suite.
A simple-protocol FETCH from a BINARY CURSOR sends format code 1 for every column (Postgres FE/BE protocol, Simple Query). For text/varchar/ bpchar/name/char the binary *send() output is byte-identical to text, so decoding those via the text path is already correct; the guard now skips them. Types whose binary format differs (uuid, date, jsonb, ...) are still rejected. Verified against real Postgres: DECLARE BINARY CURSOR; FETCH now decodes text/int4 columns as before. Added a text-with-binary-format positive case to the fault-injection suite; the uuid-with-binary-format rejection still holds.
json_send() and xml_send() emit raw text bytes with no header, so their binary output equals their text output and a BINARY CURSOR FETCH over a json or xml column must decode via the text path, not be rejected. jsonb stays excluded (jsonb_send prepends a version byte). Verified against real Postgres.
|
CI status: 281 jobs passed on build 69624. The one hard failure is The diff is green where it actually ran: |
…C decode (#34429) ## Repro A mock Postgres server sends a binary NUMERIC column (OID 1700, format=1) whose base-10000 digit word is `10000` (wire allows any u16; valid range is 0..9999): ``` panic: assertion failed: N == 0 || val < 10u64.saturating_pow(N as u32) itoa_padded<4> src/bun_core/fmt.rs:3047 parse_binary_numeric src/sql_jsc/postgres/DataCell.rs ``` On release builds the debug_assert is stripped and the high digits are silently dropped: digit `10000` decodes to `""`, `32768` decodes to `"2768"`, `65535` decodes to `"5535"`. The query succeeds and the app receives a wrong numeric value. ## Cause `parse_binary_numeric` reads each u16 digit word from the wire and passes it straight to `itoa_padded::<4>`, which requires `val < 10^4`. The integer-part loop (line 1058) and the fractional-part loop (line 1101) both do this. Postgres' own `numeric_recv` checks `d < 0 || d >= NBASE` on every received digit and errors with `invalid digit in external "numeric" value`. ## Fix Range-check `digit >= 10000` at both read sites and return `InvalidBuffer`, same posture as the existing sign check a few lines up. The caller already maps every error from this function to `ERR_POSTGRES_UNSUPPORTED_NUMERIC_FORMAT`. ## Verification `test/js/sql/postgres-binary-numeric-digit-range.test.ts` drives a scripted wire server (via `test/js/sql/wire-frames.ts`, no Postgres needed) that sends digit words 10000/10001/32768/65535 in the integer part and 10000 in the fractional part. All reject with `ERR_POSTGRES_UNSUPPORTED_NUMERIC_FORMAT`; digit 9999 and a two-word valid value still decode. Fail-before on stock 1.4.0 shows the silent corruption (`32768` -> `"2768"`). The real-Postgres `postgres-binary-numeric.test.ts` suite (13 cases) still passes. Related: #33576 validates numeric header fields but not the digit words.
1 participant
What
Bun requests the binary result format for types it can binary-decode, so whatever is on the other end of the socket (the server, PgBouncer, RDS Proxy, a MitM) authors the datum bytes in a
DataRow. Several binary decode arms insrc/sql_jsc/postgres/DataCell.rstrusted those bytes and turned a wire-level violation into an ordinary-looking JS value instead of rejecting it. A lying peer could inject data that no query produced, indistinguishable from a real column value.The
int4arm already rejected a wrong length (ERR_POSTGRES_UNSUPPORTED_INTEGER_SIZE); the rest were inconsistent. This makes the binary arms validate the datum against the declared type before decoding, the same way.Cases fixed
float8datum len 4NaNERR_POSTGRES_INVALID_BINARY_DATAfloat4datum len != 4parse_binary_int4ERR_POSTGRES_INVALID_BINARY_DATAtimestamp/timestamptzdatum len 4Invalid DateERR_POSTGRES_INVALID_BINARY_DATAboollen 0falseERR_POSTGRES_INVALID_BINARY_DATAboolvalue 2falseERR_POSTGRES_INVALID_BINARY_DATAint4[]element length prefix != 4Int32ArrayERR_POSTGRES_INVALID_BINARY_DATAtime= 2^63-1 usERR_POSTGRES_INVALID_TIME_FORMATtime= -1 usERR_POSTGRES_INVALID_TIME_FORMATnumericndigits=0 + trailing bytes"0"ERR_POSTGRES_UNSUPPORTED_NUMERIC_FORMATnumericdscale < 0ERR_POSTGRES_UNSUPPORTED_NUMERIC_FORMATuuidsent with binary format codeERR_POSTGRES_UNKNOWN_FORMAT_CODEFor the last case: a binary format code on a type Bun has no binary decoder for was silently reinterpreted as text, exposing the raw bytes (with embedded NULs / invalid UTF-8) as a string. A compliant server never sends format=1 for such a type, so
put_implnow rejects it.Cause
The binary decode arms used
if binary && bytes.len() == N { decode } else { text-parse }, so a binary datum of the wrong length fell through to the text parser.boolonly checkedbytes[0] == 1.from_bytes_typed_arraywalked a fixed element stride without reading the per-element length prefix.parse_binary_numericreturned early onndigits == 0and readdscaleas a signedi16without validating it or the total length.timepassed the raw microsecond count toPostgres__formatTimewith no range check.Fix
src/sql_jsc/postgres/DataCell.rs: each binary arm validates width (float8=8,float4=4,timestamp/timestamptz=8,bool=1 with value in {0,1}),parse_binary_numericrejectsndigits < 0,dscale < 0, and any trailing bytes (len != 8 + ndigits*2),from_bytes_typed_arraychecks each element's length prefix equals the element size,time/timetzrange-check microseconds to[0, 24h], andput_implrejects a binary format code on a type that is not inis_binary_format_supported.Verification
test/js/sql/postgres-binary-datum-validation.test.tsdrives a scripted Postgres v3 wire server (viatest/js/sql/wire-frames.ts, no Postgres needed) that declaresformat=1per column and sends each malformed datum. All 10 malformed cases pass only with the fix (fail-before confirmed by stashingsrc/); 4 well-formed datums still decode. The real-Postgres binaryNUMERICandTIMESTAMPsuites and the binaryint4[]bounds suite still pass.