sql(postgres): roll back write buffer when Bind encoding throws mid-message - #34732
sql(postgres): roll back write buffer when Bind encoding throws mid-message#34732robobun wants to merge 3 commits into
Conversation
…essage write_bind serialises parameter values straight into the connection's outgoing wire buffer, calling back into JS for each value (coerce / toString / valueOf). The Bind message's Int32 length is written as a zero placeholder and only patched once every parameter has been encoded. If a parameter's valueOf()/toString() throws mid-loop, the query rejected with the user's error but the torn 'B\0\0\0\0...' prefix stayed in the buffer; the next query's frames were appended after it and flushed together. A real PostgreSQL server answers that with 'invalid message length' and drops the connection, so one buggy/hostile parameter poisoned the pooled connection for every in-flight and subsequent query. Add WriterContext::truncate and NewWriter::atomically(), which snapshots the buffer offset, runs the batch writer, and truncates back on Err. The three batch entry points (bind_and_execute, prepare_and_query_with_signature, parse_and_bind_and_execute) are wrapped so every caller gets the rollback without per-call-site bookkeeping. MySQL was already immune: it converts every parameter before touching the writer.
|
Reproduced with a byte-capturing mock server: on the unfixed build the bytes after the prepare round-trip start Overlaps with the supporting rollback in #34708 (different primary bug, call-site approach); whichever merges first leaves the other a small rebase. CI (build #75858, 0fe017f): the new test and every other
Ready for review; needs a maintainer to merge past the unrelated red lanes. |
|
Updated 5:24 PM PT - Jul 19th, 2026
❌ @robobun, your commit 0fe017f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34732That installs a local version of the PR into your bun-34732 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe PostgreSQL writer now supports truncation and atomic rollback. Extended-query requests use atomic encoding for Parse, Bind, Execute, FLUSH, and SYNC messages. Tests cover throwing PostgreSQL atomic writes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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
🤖 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 `@test/js/sql/postgres-bind-throw-torn-frame.test.ts`:
- Around line 1-13: Trim the opening comment in the fault-injection test to the
established shared three-line banner, removing the extended write_bind mechanism
explanation. Apply the same cleanup to the additional commentary blocks around
the referenced test sections, preserving only the shared banner and leaving test
behavior unchanged.
🪄 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: 02db2848-f051-4b9d-898b-55a7c8ffc1b3
📒 Files selected for processing (5)
src/sql/postgres/protocol/ArrayList.rssrc/sql/postgres/protocol/NewWriter.rssrc/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/js/sql/postgres-bind-throw-torn-frame.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate, but there is overlap (already noted in the PR body). #34708's primary fix is rejecting out-of-range int4 values; it adds a mark/rollback pair at each of the six call sites as a supporting change so its new |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
pgReadFrontendMessages returns the unconsumed tail so a frame split across TCP segments survives; the mock was discarding it and clearing pending, which would hang the test if a client batch straddled two data events.
|
Alternate approach on branch |
There was a problem hiding this comment.
LGTM — the earlier pgReadFrontendMessages leftover-carry nit is addressed and the rollback logic checks out.
What was reviewed:
truncateis symmetric withoffset()for both implementors (ArrayListCtx→Vec::truncate; connectionWriter→byte_list.truncate(head + offset)matchingOffsetByteList::len() = byte_list.len() - head).- All
write_bindcall paths route through one of the three wrapped batch functions; the closure wrap is indent-only on the success path. - Test now carries the
pgReadFrontendMessagesleftover acrossdataevents and accumulatesreceivedfrom the incoming chunk, matching the sibling mock backends.
Extended reasoning...
Overview
Adds WriterContext::truncate and a NewWriter::atomically snapshot/rollback helper, then wraps the three Postgres batch writers (bind_and_execute, prepare_and_query_with_signature, parse_and_bind_and_execute) so that when a JS parameter coercion throws mid-write_bind, the half-written B\\0\\0\\0\\0… frame is truncated out of the write buffer instead of being flushed ahead of the next query. A byte-capturing mock-backend test asserts the wire framing directly.
Security risks
None. This is error-path cleanup on the client's own outgoing buffer; no new parsing of untrusted server bytes, no auth/TLS/permission changes.
Level of scrutiny
Moderate. It touches the Postgres extended-query serialisation path, but the diff is structurally conservative: the three function bodies are wrapped in a closure with no logic changes on the success path; the new code runs only on Err. I verified:
- The postgres
WriterContexttrait has exactly two implementors (ArrayListCtx, connectionWriter), both updated. The MySQL and js_printerWriterContextare unrelated traits. OffsetByteList::len()isbyte_list.len() - head, so the connectionWriter::truncatecomputingbyte_list.truncate(head + offset)restores exactly the stateoffset()snapshotted (assumingheadis unchanged across the batch, which it is —headonly advances viaconsume()after a socket flush, not during message encoding).write_bindis called only from the three wrapped functions, and those three cover all six external call sites (4 inPostgresSQLConnection::advance, 2 inPostgresSQLQuery::run).
Other factors
- My prior inline nit (discarding the
pgReadFrontendMessagesleftover and unconditionally resettingpending) was fixed in 0fe017f; the mock now matches the sibling callers inpostgres-datarow-overrun.test.tsandpostgres-split-prepare-reorder-fixture.ts. - CodeRabbit's comment-length nit was addressed and marked resolved.
- PR body has fail-before (
tornAt: 104, head: "42 00 00 00 00 …") / pass-after evidence on both ASAN debug and release. - The noted overlap with #34708 is a merge-order coordination question, not a correctness concern for this change; whichever lands first leaves the other a small rebase, and this PR's centralised-in-the-writer approach is the cleaner of the two.
1 participant
Repro
Against a real PostgreSQL 15, the second (innocent) query rejects with
ERR_POSTGRES_CONNECTION_CLOSEDand the server log readsinvalid message length. Against a byte-capturing mock server, the first bytes after the prepare round-trip are42 00 00 00 00 00 50 73 65 6c 65 63 74 …: a Bind message declaring length 0 (below the 4-byte minimum), followed by the partial portal/statement name, then the next query's frames.Cause
write_bindinsrc/sql_jsc/postgres/PostgresRequest.rsstreams parameter values directly intoconnection.write_buffer, calling back into JS for each value (coerce::<i32>,to_number,BunString::from_js,json_stringify_fast, and the binding iterator's getters). The Bind length field is written as a zero placeholder and patched once every parameter has been encoded. Every JS callback can throw; the error is propagated via?, but nothing truncates the already-appended bytes, so the tornB\0\0\0\0…prefix stays in the buffer and is flushed ahead of the next query's frames.The MySQL path was already immune by construction:
bind_and_execute_implconverts every parameter to a nativeVec<Value>before touching the writer.Fix
Add
WriterContext::truncateand aNewWriter::atomically(|w| …)helper that snapshotsoffset(), runs the batch body, and truncates back to the snapshot onErr. The three batch entry points that reachwrite_bindare wrapped:bind_and_executeprepare_and_query_with_signatureparse_and_bind_and_executeso every caller (4 sites in
PostgresSQLConnection::advance, 2 inPostgresSQLQuery::run) gets the rollback without per-call-site mark/restore pairs, and any future caller of these functions is covered.#34708 (int4 overflow) includes an equivalent rollback at each of the six call sites as a supporting change; this PR centralises it in the writer instead and adds a test that targets the throwing-
valueOf/toStringcase specifically. Either PR merging first leaves the other a minor rebase.Verification
test/js/sql/postgres-bind-throw-torn-frame.test.tsdrives a mock backend that records every raw byte the client sends after startup, then:select ${evil}::int4whereevil.valueOf()/evil.toString()throws; asserts the query rejects with the user's error;select ${1}::int4on the samemax: 1connection; asserts it resolves;On the unfixed build both tests fail with
{ tornAt: 104, head: "42 00 00 00 00 00 50 73 65 6c 65 63 74 20 24 31" }(the torn Bind followed by the prepared statement name); with the fix both pass.[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file