Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/sql/mysql/MySQLQuery.zig
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@
bun.default_allocator.free(params);
}
while (try iter.next()) |js_value| {
if (i >= params.len) {
// The binding array yielded more values than the prepared statement
// expects. This can happen when the user-supplied array is mutated (e.g.
// from an index getter) between signature generation and binding. Fail
// loudly instead of writing past the end of `params`/`param_types`.
return error.WrongNumberOfParametersProvided;
}

Check failure on line 33 in src/sql/mysql/MySQLQuery.zig

View check run for this annotation

Claude / Claude Code Review

bind() error leaves stale 4-byte packet header in connection write_buffer

When `bind()` returns the new `error.WrongNumberOfParametersProvided`, the 4-byte zero placeholder header that `writer.start(0)` already appended to `connection.#write_buffer` is never rolled back — the next query on this pooled connection will be prefixed with a bogus `[0,0,0,0]` packet and the wire protocol desyncs. The missing rollback is pre-existing (it already affects `Value.fromJS`/`iter.next()` failures), but this PR adds two new error returns that flow through it and the fix is trivial
Comment thread
claude[bot] marked this conversation as resolved.
const param = execute.param_types[i];
params[i] = try Value.fromJS(
js_value,
Expand All @@ -38,6 +45,12 @@
return error.InvalidQueryBinding;
}

if (i != params.len) {
// Fewer values than the prepared statement expects; the remaining slots
// would be uninitialized.
return error.WrongNumberOfParametersProvided;
}

this.#status = .binding;
execute.params = params;
}
Expand Down
44 changes: 44 additions & 0 deletions test/js/sql/sql-mysql-bind-oob.fixture.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Reproducer for an out-of-bounds write in MySQLQuery.bind().
//
// Signature.generate() and bind() each create a fresh iterator over the
// user-supplied params array. If an index getter mutates the array so that
// the second iteration is longer than the first, bind() would index past the
// `params` / `param_types` buffers it sized based on the first iteration.
//
// Without the bounds check this panics in debug builds (index out of bounds)
// and is a silent heap overflow in release builds.

import { SQL } from "bun";

const url = process.env.MYSQL_URL;
if (!url) throw new Error("MYSQL_URL is required");

const tls = process.env.CA_PATH ? { ca: Bun.file(process.env.CA_PATH) } : undefined;
const sql = new SQL({ url, tls, max: 1 });

try {
// Prime the prepared-statement cache so the next call with the same
// signature goes straight to bindAndExecute without re-preparing.
await sql.unsafe("select ? as x", [1]);

const values: number[] = [1];
let fired = 0;
Object.defineProperty(values, "0", {
enumerable: true,
configurable: true,
get() {
if (fired++ === 0) {
for (let i = 0; i < 100; i++) values.push(1);
}
return 1;
},
});

const result = await sql.unsafe("select ? as x", values).then(
rows => ({ ok: true, rows }),
err => ({ ok: false, code: err?.code, message: String(err?.message ?? err) }),
);
console.log(JSON.stringify(result));
} finally {
await sql.close();
}
32 changes: 31 additions & 1 deletion test/js/sql/sql-mysql.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { SQL, randomUUIDv7 } from "bun";
import { beforeAll, describe, expect, mock, test } from "bun:test";
import { bunEnv, bunRun, describeWithContainer, isDockerEnabled, tempDirWithFiles } from "harness";
import { bunEnv, bunExe, bunRun, describeWithContainer, isDockerEnabled, tempDirWithFiles } from "harness";
import net from "net";
import path from "path";
const dir = tempDirWithFiles("sql-test", {
Expand Down Expand Up @@ -817,6 +817,36 @@ if (isDockerEnabled()) {
expect(await sql.unsafe("select 1 as x")).toEqual([{ x: 1 }]);
});

test("unsafe does not OOB when the params array grows during binding", async () => {
// Signature generation and binding each iterate the user-supplied params
// array. If an index getter mutates the array so that the second
// iteration is longer than the first, bind() must not read/write past
// the param buffer it allocated based on the first iteration's length.
// Run in a subprocess so a crash doesn't take down the rest of the suite.
await using proc = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "sql-mysql-bind-oob.fixture.ts")],
env: {
...bunEnv,
MYSQL_URL: String(getOptions().url),
CA_PATH: image.name === "MySQL with TLS" ? path.join(import.meta.dir, "mysql-tls", "ssl", "ca.pem") : "",
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
expect(stderr).toBe("");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
expect(JSON.parse(stdout.trim())).toEqual({
ok: false,
code: "ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED",
message: expect.any(String),
});
expect(exitCode).toBe(0);
});

test("simple query with multiple statements", async () => {
await using sql = new SQL({ ...getOptions(), max: 1 });
const result = await sql`select 1 as x;select 2 as x`.simple();
Expand Down
Loading