sql: reject a Postgres message length below 4 as a protocol error - #32769
Conversation
Every PostgreSQL v3 backend message carries an Int32 length that includes its own 4 bytes, so a smaller value is malformed and makes the stream unframeable. The value is server-controlled, but five decoders (ReadyForQuery, ParameterStatus, CommandComplete, NotificationResponse, NegotiateProtocolVersion) validated it with debug_assert!, which aborts the whole process in any build with debug assertions enabled (debug, release-asan, release-assertions). Builds without them silently accepted the malformed length instead. Move the check into NewReaderWrap::length(), the one place every decoder reads the field, and surface it as AnyPostgresError::InvalidMessageLength (ERR_POSTGRES_INVALID_MESSAGE_LENGTH). That makes the five debug asserts and the two pre-existing copies of the same check (in ErrorResponse and PostgresRequest's unknown-message arm) redundant, so they are removed. ErrorResponse::decode_internal and decode_notice_internal become identical as a result and are merged; a NoticeResponse declaring a length below 4 is now rejected like every other message, matching libpq, which treats it as a sync loss for all message types.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 27 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 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 credits. 🚦 How do rate 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 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 (12)
Comment |
|
Updated 10:25 AM PT - Jun 26th, 2026
❌ @robobun, your commit c453621 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32769That installs a local version of the PR into your bun-32769 --bun |
|
Reproduced with The diff is done and both rounds of inline review feedback are addressed. All 23 debian x64-asan test lanes passed on both of the last two full builds (65026 and 65041); those are the lanes where the removed
I have already spent one |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — moving the < 4 check into the shared length() helper is the right chokepoint, and the fallout deletions are all provably dead. The one inline note is a cosmetic nit.
Extended reasoning...
Overview
This PR centralizes Postgres v3 message-length validation in NewReaderWrap::length(), the single helper every decoder reads the length field through. A declared length below 4 (which is unframeable per the protocol — the length includes its own 4 bytes) now returns AnyPostgresError::InvalidMessageLength instead of either tripping a debug_assert! (debug/asan builds → process abort) or being silently accepted (release builds). The fallout is mechanical: five tautological debug_assert!(length >= 4) calls removed, two now-redundant call-site checks deleted, and decode_notice_internal collapsed into decode_internal since they became identical. Net -23 lines in src/. A new fault-injection test file drives each reachable decoder with a lying length field plus the length-exactly-4 boundary as a positive control; pgRaw in the shared test helpers gains an optional declaredLength parameter with a backward-compatible default.
Security risks
None introduced — this is strictly a hardening change. Previously a malicious/buggy pooler, proxy, or MITM on the connection path could crash any debug-assertion build by sending a single malformed length byte (server-controlled DoS), or in release builds cause the malformed length to be silently trusted. Now it surfaces as ERR_POSTGRES_INVALID_MESSAGE_LENGTH on the affected connection only, matching libpq's sync-loss treatment. The one deliberate behavior change (NoticeResponse with length < 4 is now rejected rather than decoded as empty) is correct: an unframeable length is fatal regardless of message type, and the length-exactly-4 boundary remains accepted (covered by a test).
Level of scrutiny
Low-to-moderate. The fix is a 4-line addition at a single chokepoint; everything else is deletion of code that the new invariant makes provably unreachable. length() already propagated errors via ? at every call site, so the new error path needs no caller changes. The saturating_sub(4) → - 4 change inside length() itself is safe because the guard immediately precedes it. No CODEOWNERS paths are touched.
Other factors
robobun independently reproduced the panic on the unfixed debug build and the silent-accept failures on release, and confirmed all 9 new tests plus 37 neighboring wire-protocol tests pass with the fix. The bug-hunter's only finding is a cosmetic nit (two leftover saturating_sub(4) at call sites that are now equivalent to - 4) with zero runtime effect — not a blocker.
…ast 4 The saturating subtraction at these two call sites could only saturate on a length below 4, which length() now rejects before returning.
There was a problem hiding this comment.
LGTM — thanks for addressing the nit in 1fa2595; the inline note on the signed high-bit case is pre-existing and non-blocking.
Extended reasoning...
Overview
Centralizes the Postgres v3 "message length must be ≥ 4" check in NewReaderWrap::length(), the one helper every decoder reads the length through. Five debug_assert!(length >= 4) calls (which aborted the process in debug/asan builds on server-controlled input, and compiled out in release) become tautologies and are removed; two duplicate runtime checks are removed; decode_notice_internal is deleted because it converged with decode_internal. Net -23 lines in src/. A new fault-injection test file drives every reachable decoder with a lying length field plus the length=4 boundary as a positive control, and pgRaw() in the shared test helper grows a declaredLength override. The one-line change in PostgresSQLConnection.rs swaps the deleted notice decoder for the shared one.
Security risks
The change is strictly hardening: it converts a debug_assert! on server-controlled input (process abort) and silent acceptance (release) into a per-connection ERR_POSTGRES_INVALID_MESSAGE_LENGTH. No new parsing surface is added — only an early-reject. The deliberate behavior change for NoticeResponse (length < 4 now rejected instead of decoded as empty) is correct: an unframeable length is fatal regardless of message type, and this matches libpq. The remaining inline observation (high-bit-set length passes the unsigned < 4 check) is pre-existing, byte-for-byte unchanged by this PR, and doesn't open a new vector — ensure_capacity is a pure length comparison with no allocation.
Level of scrutiny
Wire-protocol parsing warrants care, but the diff is mechanical: one 4-line guard added at a natural choke point, and downstream removals of code that the guard makes provably dead. A conforming server never sends length < 4, so well-formed traffic is unaffected. The 9 new tests exercise each decoder and the boundary, and the PR description reports the neighboring 37 wire-suite tests still pass.
Other factors
My earlier nit (saturating_sub(4) left at two call sites) was addressed in 1fa2595 and the thread is resolved. No CODEOWNERS cover these paths. No remaining references to the deleted decode_notice_internal. The author's choice to leave saturating_sub(4) in untouched files (DataRow/RowDescription/ParameterDescription) as out-of-scope is reasonable for a focused bugfix.
The protocol defines the length as a signed Int32, so a wire value in 0x80000000..=0xFFFFFFFF is negative and just as unframeable as one below 4. libpq's msgLength < 4 comparison is on the signed type and rejects both halves; PostgresInt32 is u32 here, so the high half needs its own clause. A server declaring a length of -1 now fails the connection with InvalidMessageLength instead of stalling on the wait-for-more-bytes path.
There was a problem hiding this comment.
LGTM — both earlier review rounds are addressed, and the centralized length() check with the new fault-injection coverage looks correct.
Extended reasoning...
Overview
This PR centralizes Postgres v3 message-length validation into NewReaderWrap::length(), the single helper every backend-message decoder reads the length through. A declared length below 4 (or with the sign bit set) now returns AnyPostgresError::InvalidMessageLength instead of tripping a debug_assert! (debug builds) or being silently accepted (release builds). The fallout is purely subtractive: five debug_assert!(length >= 4) calls become tautologies and are removed, two pre-existing copies of the same runtime check (in ErrorResponse::decode_internal and the unknown-message arm of on_data) become dead and are removed, and decode_notice_internal collapses into the now-identical decode_internal. Net -23 lines in src/ across 10 source files, plus a new 127-line fault-injection test and a small extension to the shared pgRaw helper.
Security risks
None introduced — this is a hardening change that makes the wire parser strictly more restrictive against server-controlled input. The length field comes from whatever sits on the connection path (server, pooler, proxy, tunnel), and the change converts an assertion on untrusted input into a proper protocol error that fails the one connection. The added expected > i32::MAX as u32 clause closes the signed-vs-unsigned gap with libpq's reference check. No new code paths accept input that was previously rejected; the only behavior change in the accept direction is none.
Level of scrutiny
Moderate-low. While this touches the Postgres wire-protocol reader, the actual edits are mechanical: a 6-line guard added at one chokepoint, and a series of 1–2-line deletions of checks that guard now subsumes. The one deliberate semantic change — a NoticeResponse declaring length < 4 is now rejected rather than decoded as empty — is correct (an unframeable length desyncs the stream regardless of message type, and libpq rejects it the same way) and is explicitly tested alongside the length-exactly-4 positive boundary. I confirmed decode_notice_internal has no remaining callers after its removal.
Other factors
I reviewed two earlier revisions of this PR; both inline threads are resolved with follow-up commits (1fa2595 replaced the dead saturating_sub(4) calls with plain subtraction, 35bce47 added the sign-bit rejection and a -1 test case). The bug-hunting system found nothing on the current head. The new test file exercises every reachable decoder via a table-driven matrix plus the in-flight-query path for CommandComplete, and the PR description documents that all 9 cases pass on the fixed build and fail/abort on the unfixed one. No CODEOWNERS cover the touched paths.
There was a problem hiding this comment.
LGTM — both earlier nits addressed; the centralized length() check is a strict hardening with no effect on valid frames.
Extended reasoning...
Overview
Centralizes Postgres v3 message-length validation in NewReaderWrap::length(): a declared length < 4 or > i32::MAX (sign bit set on the wire) now returns AnyPostgresError::InvalidMessageLength instead of either debug_assert!-panicking (debug builds) or being silently accepted (release builds). Five now-tautological debug_assert!s and two duplicate call-site checks are removed; decode_notice_internal collapsed into decode_internal; NoticeResponse dispatch updated accordingly. Net −23 lines in src/. A new fault-injection test file drives each reachable decoder with malformed length fields (3, 0, −1) plus a length-exactly-4 positive control, and pgRaw() in wire-frames.ts gains an optional declaredLength parameter (backward-compatible default).
Security risks
None introduced. The change is strictly defensive: server-controlled input that previously aborted the process (debug) or was trusted (release) now surfaces as ERR_POSTGRES_INVALID_MESSAGE_LENGTH on that connection. For every valid frame a real Postgres server emits (length ∈ [4, i32::MAX]), length() behaves identically to before — same ensure_capacity((expected - 4) as usize) call, same return value. The one deliberate behavior change (a NoticeResponse declaring length < 4 now fails instead of decoding as empty) aligns with libpq and the protocol spec, since such a frame is unframeable.
Level of scrutiny
Moderate. length() sits on the hot path of every Postgres backend message, but the diff to it is a single guarded early-return on provably-invalid values, with the rest of the function unchanged for valid input. The decoder edits are mechanical removals of dead asserts/checks. No CODEOWNERS cover these paths.
Other factors
I previously left two inline comments on this PR (dead saturating_sub guards; signed-vs-unsigned parity with libpq); both were addressed in 1fa2595 and 35bce47 and marked resolved. The bug-hunting system found no issues on the current revision. The one CI failure (sql-mysql.helpers.test.ts on aarch64) is in MySQL test code this PR does not touch and appears to be the flake the ci: retrigger commit is chasing.
2 participants
A Postgres backend (or a proxy / pooler in between) that sends a message whose 4 byte length field declares a value below 4 makes Bun's SQL client abort the whole process in any build with debug assertions enabled (debug,
release-asan,release-assertions):In release builds (debug assertions off) the
debug_assert!compiles out and the malformed length is silently accepted instead.Reproduction
A mock server that completes the handshake and then sends a
ReadyForQuery(Z) whose length field says 3:Cause
Every PostgreSQL v3 message carries a signed
Int32length that includes its own 4 bytes, so a value below 4 (or a negative one, i.e. the sign bit set on the wire) is malformed and makes the stream unframeable: there is no way to skip to the next message. libpq treatsmsgLength < 4as a sync loss for every message type, and because its comparison is on the signed type it rejects both halves.The value is server controlled, but five decoders validated it with
debug_assert!:ReadyForQuery,ParameterStatus,CommandComplete,NotificationResponse, andNegotiateProtocolVersion. Three other sites already returnedAnyPostgresError::InvalidMessageLengthfor the same condition (ErrorResponse::decode_internal,Authentication, and the unknown-message arm inPostgresRequest::on_data).Fix
Move the check into
NewReaderWrap::length(), the one helper every decoder reads the length through. A value below 4, or one with the sign bit set (PostgresInt32isu32, so the negative half of the protocol's signedInt32shows up as values abovei32::MAX), now returnsAnyPostgresError::InvalidMessageLength, which surfaces to JS asERR_POSTGRES_INVALID_MESSAGE_LENGTHon the failed connection. Fallout, all in this PR:debug_assert!(length >= 4)became tautologies and are removed.ErrorResponse::decode_internaland the unknown-message arm) became dead and are removed.ErrorResponse::decode_internalbecame identical todecode_notice_internal, so the latter is deleted andNoticeResponseuses the shared decoder.One deliberate behavior change: a
NoticeResponsedeclaring a length below 4 previously decoded as an empty notice; it is now rejected like every other message, matching libpq. ANoticeResponsewith an empty body (length exactly 4) is still accepted. The decoders that had no check at all (DataRow,RowDescription,CopyData,ParameterDescription) are covered by the shared check now too.NegotiateProtocolVersioncurrently has no dispatch arm (avbyte falls through to the unknown-message path), so its assert was unreachable; it is removed for consistency but cannot be exercised by a test.Out of scope: a server that declares an in-range length and never sends the bytes still parks the connection in the wait-for-more-data path until a timeout. That needs a max-message-size cap and is independent of length-field validity.
Net -23 lines in
src/.Verification
test/js/sql/postgres-invalid-message-length.test.tsdrives each reachable decoder with a frame whose length field lies (3, 0, and -1), plus the length-exactly-4 boundary as the positive control.USE_SYSTEM_BUN=1): 7 of 10 cases fail because the malformed length is silently accepted and the connection times out.bun bd test): all 10 pass, along with the 37 tests in the neighboring postgres wire suites (wire-frames,postgres-binary-array-bounds,sql-connect-error-reporting,sql-close-pending-connection,sql-onconnect-onclose-throw).