sql(mysql): bounds-check params in bind() when values array is mutated#29886
Conversation
MySQLQuery.bind() allocates a params buffer sized to the signature that was captured when the statement was first prepared, then iterates a fresh QueryBindingIterator over the user-supplied values array. If the array is mutated between signature generation and binding (e.g. via an index getter that pushes, or between prepare and execute), the second iteration can be longer than the first, causing an out-of-bounds read of execute.param_types and an out-of-bounds heap write into params. PostgresRequest.writeBind already guards this case; MySQL did not. Fail with ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED when the iterator yields a different count than the statement expects, instead of writing past the allocation (or leaving trailing slots uninitialized in the shrink case).
|
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:
Walkthroughbind now enforces two-way parameter-count validation and reorders binding to occur before packet header emission; tests added reproduce an iterator-expansion out-of-bounds case and verify rejection and protocol usability after failure. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 8:03 AM PT - Apr 29th, 2026
❌ @robobun, your commit 1cc6b1d has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29886That installs a local version of the PR into your bun-29886 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…sync bindAndExecute previously called writer.start(0) (which appends a 4-byte placeholder header to the connection's write buffer) before bind(). If bind() failed — now reachable via the new param-count guard, but already reachable via Value.fromJS / iterator errors — those bytes were left in the buffer and prefixed the next query on the pooled connection, causing a protocol desync (server responds 1047 Unknown command). bind() never touches the writer, so run it first. Also defer clearing need_to_send_params until after bind() succeeds so a failed bind doesn't skip param types on the next execution. The regression fixture now also asserts the connection remains usable after the rejected bind.
![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 the current code and only fix it if needed.
Inline comments:
In `@src/sql/mysql/MySQLQuery.zig`:
- Around line 73-76: The code sets statement.execution_flags.need_to_send_params
= false before the execute packet is fully serialized which can leave the
statement in an inconsistent state if writer.start(), execute.write(), or
packet.end() fails; move the mutation of
statement.execution_flags.need_to_send_params so it only happens after the
packet is successfully completed (i.e., after packet.end() returns), or
alternatively keep the current write sequence but wrap the flag change behind a
successful packet serialization by using a try on packet.end() and then set
need_to_send_params = false (or use a defer/rollback to restore the flag on
error); update the block around this.bind(...), writer.start(...),
execute.write(...), and packet.end() to ensure need_to_send_params is only
cleared on successful packet emission.
In `@test/js/sql/sql-mysql.test.ts`:
- Around line 836-841: The test currently asserts raw stderr is empty after
launching the subprocess (proc, stdout, stderr, exitCode), which fails on
ASAN/debug shards; update the assertion to first split stderr into lines and
filter out the known ASAN startup warning (use .split(/\r?\n/).filter(line =>
!line.startsWith("WARNING: ASAN interferes")) or similar), then assert the
filtered result is empty (e.g., join back and expect("") or
expect(filteredLines).toEqual([])), keeping the rest of the await Promise.all
for proc.stdout.text(), proc.stderr.text(), proc.exited 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: 7d732450-5ae4-4b17-a851-4d9c25f4a5f8
📒 Files selected for processing (3)
src/sql/mysql/MySQLQuery.zigtest/js/sql/sql-mysql-bind-oob.fixture.tstest/js/sql/sql-mysql.test.ts
The test needs a MySQL server but sql-mysql.test.ts is fully docker-gated; in environments without a docker daemon it runs 0 tests. Move the regression to its own file that uses describeWithContainer when docker is available and otherwise falls back to MYSQL_URL / a local 127.0.0.1:3306 instance (distinguishing 'couldn't connect' from 'connected then crashed' via a CONNECTED marker printed after the priming query).
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 `@test/js/sql/sql-mysql-bind-oob.test.ts`:
- Around line 70-73: The current skip logic in the test uses
stdout.startsWith("CONNECTED") to bail out, which can incorrectly skip tests
when an explicit MYSQL_URL was provided; change the condition so the test only
skips when stdout does not start with "CONNECTED" AND process.env.MYSQL_URL is
unset/empty. Locate the check using stdout.startsWith("CONNECTED") (and the url
variable) and update it to require absence of MYSQL_URL before returning so
explicit fixture configuration still fails the test instead of passing silently.
🪄 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: 352c1871-71fe-484e-9ab7-180d0d6e6350
📒 Files selected for processing (2)
test/js/sql/sql-mysql-bind-oob.fixture.tstest/js/sql/sql-mysql-bind-oob.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/sql/sql-mysql-bind-oob.test.ts (1)
68-70:⚠️ Potential issue | 🟡 MinorDo not silently skip when
MYSQL_URLis explicitly set.On Line [68]-Line [70], the test can pass even when a user-provided
MYSQL_URLis broken or regressed. That hides real failures in configured local runs.Proposed fix
- if (!stdout.startsWith("CONNECTED")) { - console.warn("sql-mysql-bind-oob: no MySQL reachable at " + url + "; skipping assertions"); - return; - } + if (!stdout.startsWith("CONNECTED")) { + if (process.env.MYSQL_URL) { + throw new Error( + "sql-mysql-bind-oob: MYSQL_URL was provided, but fixture never reached CONNECTED\n" + + `stdout:\n${stdout}\n` + + `stderr:\n${stderr}`, + ); + } + console.warn("sql-mysql-bind-oob: no MySQL reachable at " + url + "; skipping assertions"); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/sql/sql-mysql-bind-oob.test.ts` around lines 68 - 70, The test currently silently returns when stdout doesn't start with "CONNECTED", hiding failures when a user-supplied MYSQL_URL is set; change the early-return logic around stdout and url so that if process.env.MYSQL_URL (the user-provided MYSQL_URL) is present the test fails loudly (throw an Error or use an assertion) instead of console.warn/return, and only skip with the existing warning when MYSQL_URL is not set; update the block that references stdout and url to check process.env.MYSQL_URL and call assert.fail or throw new Error("MYSQL_URL provided but MySQL not reachable: " + url) when it is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/sql/sql-mysql-bind-oob.test.ts`:
- Around line 68-70: The test currently silently returns when stdout doesn't
start with "CONNECTED", hiding failures when a user-supplied MYSQL_URL is set;
change the early-return logic around stdout and url so that if
process.env.MYSQL_URL (the user-provided MYSQL_URL) is present the test fails
loudly (throw an Error or use an assertion) instead of console.warn/return, and
only skip with the existing warning when MYSQL_URL is not set; update the block
that references stdout and url to check process.env.MYSQL_URL and call
assert.fail or throw new Error("MYSQL_URL provided but MySQL not reachable: " +
url) when it is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8e0197b-76a9-42b6-a5a6-5245dde762ce
📒 Files selected for processing (1)
test/js/sql/sql-mysql-bind-oob.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/sql/mysql/MySQLQuery.zig:73— Pre-existing, but worth a follow-up: the PR description's trigger #2 (array mutated between prepare and execute) flows throughMySQLRequestQueue.advance, and on that path the newerror.WrongNumberOfParametersProvidedcauses the user'sawaitto hang forever instead of rejecting.run()'serrdefer(JSMySQLQuery.zig:277-280) marks the query.failbefore re-throwing, thenconnection.onError → rejectWithJSValuesees status already.failat line 237 and returns early without invokingonQueryRejectFn. This double-fail()short-circuit predates this PR (it already swallowedValue.fromJS/InvalidQueryBindingerrors on the same path) and the fix lives inJSMySQLQuery.zig, not in the files touched here — flagging only because the description cites trigger #2 as covered when in practice it trades the heap overflow for a hung promise.Extended reasoning...
What the bug is
The PR lists two triggers for the OOB. Trigger #1 (cached statement, synchronous
doRunpath) is fixed and tested by the new fixture. Trigger #2 — "async mutation between prepare and execute" — takes a different path: the request is enqueued, the server replies toCOM_STMT_PREPARE, andcheckIfPreparedStatementIsDonecallsqueue.advance(connection), which re-invokesrequest.run()to bind and execute. Whenbind()returns the newerror.WrongNumberOfParametersProvidedon that path, the user's promise is never settled.The root cause is a double state transition.
JSMySQLQuery.run()registers:errdefer { this.#thisValue.downgrade(); _ = this.#query.fail(); // sets #status = .fail }
at JSMySQLQuery.zig:277-280, then re-throws
error.JSError. Back inMySQLRequestQueue.advance(line 109-118) the catch callsconnection.onError(request, err)→JSMySQLConnection.onError(726-736) →tryTakeException()→request.rejectWithJSValue(...). ButrejectWithJSValue(JSMySQLQuery.zig:237) does:if (!this.#query.fail()) { return; }
and
MySQLQuery.fail()returnsfalsewhen#statusis already.fail. SorejectWithJSValuereturns early before invokingonQueryRejectFn.advancethen discards the request from the queue and derefs it; the JS promise is left pending forever.Step-by-step proof (trigger #2)
sql.unsafe("select ? as x", values)with a fresh signature →doRun→run→runPreparedQuerysendsCOM_STMT_PREPARE(statement.status = .parsing), returns OK, request is enqueued. The user's array is captured by reference asbinding_value.- User code mutates
values.push(...)while the round-trip is in flight. - Server replies prepare-OK →
checkIfPreparedStatementIsDone(MySQLConnection.zig:841-851) setsstatement.status = .prepared,#is_ready_for_query = true,#waiting_to_prepare = false, then callsqueue.advance(connection). advancepeeks the request:isCompleted()false,isBeingPrepared()false (statement is now.prepared),isRunning()false (#status == .pending), so it callsrequest.run(connection)(line 109).run()passes the guard at line 270, upgrades#thisValue, registers the errdefer, and callsrunQuery → runPreparedQuery. The statement is.preparedandcanPipeline()is true, sobindAndExecuteruns.bind()re-iterates the (now longer) array and ati == params.lenreturns the newerror.WrongNumberOfParametersProvided.runPreparedQuerycatches it at MySQLQuery.zig:157, throws a JS exception, returnserror.JSError.run()'s catch at 284-289 returnserror.JSError. The errdefer fires →this.#query.fail()sets#status = .fail.- Back in
advance:connection.onError(request, err)→JSMySQLConnection.onError→tryTakeException()finds the pending exception →request.rejectWithJSValue(queries_array, err_). rejectWithJSValuecallsthis.#query.fail()→ returnsfalse(already.fail) →!false→ early return without callingonQueryRejectFn.advancediscards the request (this.#requests.discard(1); request.deref();). The user'sawait sql.unsafe(...)never resolves or rejects.
Why the new test doesn't catch it
The fixture primes the statement cache with a first call so the failing call hits a
.preparedstatement synchronously insidedoRun. There,run()throws →doRun's own catch (JSMySQLQuery.zig:114) returnserror.JSErrorstraight to the JS caller beforeenqueueRequest, and the JS layer turns that into a rejection.MySQLRequestQueue.advanceis never involved, so the double-fail()path isn't exercised.Why existing code doesn't prevent it
rejectWithJSValuedeliberately guards onfail()to avoid double-rejecting an already-settled query, andrun()'s errdefer pre-emptively marks.failto keep the query from being retried. Both are individually reasonable, but together they mean any error thrown out ofrun()on theadvancepath is silently dropped. Nothing else on this path (neitheronErrornoradvance) invokes the JS rejection callback.Impact
For trigger #2 the PR converts a heap overflow into a hung
await— strictly safer, but not the "clean rejection" the description claims. In practice the array-mutation-during-prepare scenario is adversarial / pathological, so the user-visible impact is small; the more important point is that this same short-circuit already swallowsValue.fromJSerrors, getter exceptions, anderror.InvalidQueryBindingwhenever they fire during the post-prepare bind, and this PR adds two more error returns that flow through it.How to fix
This is pre-existing and lives outside the files this PR touches. Either:
- Drop
_ = this.#query.fail();fromrun()'s errdefer (JSMySQLQuery.zig:279) and letrejectWithJSValueown the.pending → .failtransition; or - Change
rejectWithJSValueto invokeonQueryRejectFneven when the status is already.fail(and rely on the JS side /isCompleted()to dedupe).
Either way it's a small follow-up; not blocking for this PR.
robobun
force-pushed
the
farm/9728add3/mysql-bind-oob
branch
from
3e94aac to
89590a3
Compare
There was a problem hiding this comment.
The force-push resolved my previous blocker — MySQLQuery.zig now contains the bounds-check + reorder and the unrelated files are gone. Deferring to a human for final sign-off since this is a memory-safety fix that reorders the prepared-statement execute path.
Extended reasoning...
Overview
The PR now touches exactly three files: src/sql/mysql/MySQLQuery.zig (the actual fix), plus a new subprocess fixture and standalone test under test/js/sql/. The Zig change adds an i >= params.len overflow guard and an i != params.len underflow guard inside bind(), moves bind() to run before writer.start(0) so a bind failure doesn't leave a stale 4-byte header in the connection write buffer, and defers need_to_send_params = false until after packet.end() succeeds. All four review threads (write-buffer desync, need_to_send_params placement, ASAN stderr filtering, and the MYSQL_URL skip narrowing) are resolved, and the botched 3e94aac commit that I flagged last round has been replaced by a clean 89590a3 — git diff f133e38..89590a3 -- src/sql/mysql/MySQLQuery.zig is now empty and the changed-files list is back to 3.
Security risks
This is a security fix: it closes a user-triggerable heap buffer overflow in the MySQL prepared-statement bind path (OOB write into a bun.default_allocator.alloc'd slice when a side-effecting array getter grows the params array between signature generation and binding). The change is strictly defensive — it adds bounds checks and reorders operations so failures don't corrupt connection state. I don't see any new attack surface introduced; the errdefer that frees params[0..i] already covers the new early-return paths correctly.
Level of scrutiny
High. This is native memory-safety code in a database driver, and the bindAndExecute reorder changes when bytes hit the wire-protocol write buffer relative to JS-observable side effects. The change has been through five review rounds including one force-push that briefly dropped the fix entirely. While I've verified the current diff is correct and self-consistent, a maintainer familiar with the MySQL connection/statement lifecycle should confirm the reorder has no unintended interaction with pipelining or the statement cache.
Other factors
The regression test exercises both the bounds check (asserts ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED) and the write-buffer reorder (asserts a follow-up query on the same pooled connection still returns [{x: 2}]), runs against the docker MySQL container in CI, and follows repo conventions for ASAN-stderr filtering and exit-code-last assertion ordering. No CODEOWNERS entry covers src/sql/mysql/.
There was a problem hiding this comment.
All prior review feedback is addressed and the current diff looks correct to me, but since this hardens a user-triggerable heap overflow and reorders packet emission in bindAndExecute, I'd like a human familiar with the MySQL driver to give it a final look.
Extended reasoning...
Overview
Three files: src/sql/mysql/MySQLQuery.zig adds two bounds checks in bind() (overflow guard inside the iterator loop, underflow guard after it) returning the pre-existing error.WrongNumberOfParametersProvided, and reorders bindAndExecute() so bind() runs before writer.start(0) and need_to_send_params is cleared only after packet.end(). Two new test files add a subprocess-based regression that exercises both the bounds check and post-failure connection usability against a real MySQL.
Security risks
The change removes a security risk — a user-triggerable heap buffer overflow via a side-effecting array index getter passed to sql.unsafe(). The fix is purely defensive (more validation, earlier failure). I see no new injection, auth, or data-exposure surface introduced. The reorder means a bind failure no longer leaves a stale 4-byte header in the pooled connection's write buffer, which was a protocol-desync risk I flagged earlier and is now covered by the fixture's follow-up query assertion.
Level of scrutiny
Moderate-to-high. The Zig change is small (~20 lines) and the logic is clear, but it sits on the prepared-statement execute path of a production database driver and changes the relative ordering of packet-header emission vs. JS value coercion. I traced bind() and confirmed it never touches the writer, so moving it earlier is safe; execute.new_params_bind_flag is captured before the flag mutation in both old and new orderings, so the success path is unchanged. Still, per the approval guidelines I should not auto-approve security-sensitive code.
Other factors
Every inline comment on this PR (mine on the write-buffer rollback and the dropped-fix force-push, CodeRabbit's on need_to_send_params ordering, ASAN stderr filtering, skip-condition narrowing, and the per-test timeout) is resolved in the current head (1451d6d). The error enum and JS error code already existed in AnyMySQLError.zig. No CODEOWNERS entry covers src/sql/. The early CI comment showed musl build-infra failures unrelated to this change. The bug-hunting system found nothing in the current revision.
oven-sh#29886) ## Problem `MySQLQuery.bind()` allocates `params = alloc(Value, execute.param_types.len)` — sized to the signature captured when the statement was prepared — and then iterates a **fresh** `QueryBindingIterator` over the user-supplied values array with no bounds check: ```zig while (try iter.next()) |js_value| { const param = execute.param_types[i]; // OOB read when i >= len params[i] = try Value.fromJS(...); // OOB heap write i += 1; } ``` `Signature.generate()` and `bind()` each call `JSArrayIterator.init()`, which re-reads `array.length`. If the user's array grows between the two iterations, `bind()` writes past the end of its allocation. The array is the user's own object in the `sql.unsafe(query, args)` path — no defensive copy is taken. Two concrete triggers (neither is guarded): 1. **Cached statement + side-effecting getter**: prime the statement cache with `sql.unsafe("select ?", [1])`. Then call it again with an array whose index-0 getter pushes onto itself. `Signature.generate` captures `length=1` before the getter fires, yields the same signature hash, and falls through to the cached `.prepared` statement → `bindAndExecute` → `bind()` re-reads `length=101` and walks off the end. 2. **Async mutation between prepare and execute**: start a query (signature generated, `COM_STMT_PREPARE` sent), mutate the array before the server responds, then `bind()` sees the new length. `PostgresRequest.writeBind` already handles this (`if (i >= len) break :brk .text`); MySQL did not. ## Repro ```js await sql.unsafe("select ? as x", [1]); // prime cache const values = [1]; let fired = 0; Object.defineProperty(values, "0", { get() { if (fired++ === 0) for (let i = 0; i < 100; i++) values.push(1); return 1; }, }); await sql.unsafe("select ? as x", values); ``` Debug build before this change: ``` panic(main thread): index out of bounds: index 1, len 1 sql.mysql.MySQLQuery.bind /src/sql/mysql/MySQLQuery.zig:27:42 ``` In release builds this is a silent heap buffer overflow. ## Fix In `bind()`, return `error.WrongNumberOfParametersProvided` when the iterator yields more values than the allocated buffer, and likewise when it yields fewer (which would leave trailing slots uninitialized before `PreparedStatement.Execute.write` iterates them). ## Verification With the fix, the repro rejects cleanly: ``` {"ok":false,"code":"ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED","message":"failed to bind and execute query"} ``` Added a subprocess-based regression test to `test/js/sql/sql-mysql.test.ts` (fixture in `test/js/sql/sql-mysql-bind-oob.fixture.ts`) so a regression shows up as a test failure rather than crashing the whole suite. --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
Problem
MySQLQuery.bind()allocatesparams = alloc(Value, execute.param_types.len)— sized to the signature captured when the statement was prepared — and then iterates a freshQueryBindingIteratorover the user-supplied values array with no bounds check:Signature.generate()andbind()each callJSArrayIterator.init(), which re-readsarray.length. If the user's array grows between the two iterations,bind()writes past the end of its allocation. The array is the user's own object in thesql.unsafe(query, args)path — no defensive copy is taken.Two concrete triggers (neither is guarded):
sql.unsafe("select ?", [1]). Then call it again with an array whose index-0 getter pushes onto itself.Signature.generatecaptureslength=1before the getter fires, yields the same signature hash, and falls through to the cached.preparedstatement →bindAndExecute→bind()re-readslength=101and walks off the end.COM_STMT_PREPAREsent), mutate the array before the server responds, thenbind()sees the new length.PostgresRequest.writeBindalready handles this (if (i >= len) break :brk .text); MySQL did not.Repro
Debug build before this change:
In release builds this is a silent heap buffer overflow.
Fix
In
bind(), returnerror.WrongNumberOfParametersProvidedwhen the iterator yields more values than the allocated buffer, and likewise when it yields fewer (which would leave trailing slots uninitialized beforePreparedStatement.Execute.writeiterates them).Verification
With the fix, the repro rejects cleanly:
Added a subprocess-based regression test to
test/js/sql/sql-mysql.test.ts(fixture intest/js/sql/sql-mysql-bind-oob.fixture.ts) so a regression shows up as a test failure rather than crashing the whole suite.