diff --git a/src/sql/mysql/MySQLQuery.zig b/src/sql/mysql/MySQLQuery.zig index 563701df5d7..fd657fb5b49 100644 --- a/src/sql/mysql/MySQLQuery.zig +++ b/src/sql/mysql/MySQLQuery.zig @@ -24,6 +24,13 @@ fn bind(this: *MySQLQuery, execute: *PreparedStatement.Execute, globalObject: *J 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; + } const param = execute.param_types[i]; params[i] = try Value.fromJS( js_value, @@ -38,6 +45,12 @@ fn bind(this: *MySQLQuery, execute: *PreparedStatement.Execute, globalObject: *J 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; } @@ -47,18 +60,21 @@ fn bindAndExecute(this: *MySQLQuery, writer: anytype, statement: *MySQLStatement if (statement.signature.fields.len != statement.params.len) { return error.WrongNumberOfParametersProvided; } - var packet = try writer.start(0); var execute = PreparedStatement.Execute{ .statement_id = statement.statement_id, .param_types = statement.signature.fields, .new_params_bind_flag = statement.execution_flags.need_to_send_params, .iteration_count = 1, }; - statement.execution_flags.need_to_send_params = false; defer execute.deinit(); + // Bind before touching the writer so a bind failure (user-triggerable via JS + // getters / param-count mismatch) doesn't leave a partial packet header in + // the connection's write buffer. try this.bind(&execute, globalObject, binding_value, columns_value); + var packet = try writer.start(0); try execute.write(writer); try packet.end(); + statement.execution_flags.need_to_send_params = false; this.#status = .running; } diff --git a/test/js/sql/sql-mysql-bind-oob.fixture.ts b/test/js/sql/sql-mysql-bind-oob.fixture.ts new file mode 100644 index 00000000000..f8038386584 --- /dev/null +++ b/test/js/sql/sql-mysql-bind-oob.fixture.ts @@ -0,0 +1,52 @@ +// 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]); + // Marker so the test harness can distinguish "couldn't connect" from + // "connected then crashed" when no docker-managed MySQL is available. + console.log("CONNECTED"); + + 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) }), + ); + + // The connection must still be usable after the bind failure; a partial + // packet header left in the write buffer would desync the protocol here. + const after = await sql.unsafe("select ? as x", [2]); + + console.log(JSON.stringify({ result, after })); +} finally { + await sql.close(); +} diff --git a/test/js/sql/sql-mysql-bind-oob.test.ts b/test/js/sql/sql-mysql-bind-oob.test.ts new file mode 100644 index 00000000000..700a146c5e5 --- /dev/null +++ b/test/js/sql/sql-mysql-bind-oob.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, describeWithContainer, isDockerEnabled } from "harness"; +import path from "path"; + +// Regression: MySQLQuery.bind() allocates `params` sized to the prepared +// statement's signature and then iterates a *fresh* iterator over the user's +// values array. If that array grew between signature generation and bind +// (e.g. via an index getter with side effects), bind() would walk off the +// end of the allocation. With the fix it rejects with +// ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED and the connection stays +// usable. + +const fixture = path.join(import.meta.dir, "sql-mysql-bind-oob.fixture.ts"); + +async function runFixture(url: string, caPath = "") { + await using proc = Bun.spawn({ + cmd: [bunExe(), fixture], + env: { ...bunEnv, MYSQL_URL: url, CA_PATH: caPath }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +function assertFixtureOutput(stdout: string, stderr: string, exitCode: number) { + const filteredStderr = stderr + .split(/\r?\n/) + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + expect(filteredStderr).toBe(""); + const lines = stdout.trim().split(/\r?\n/); + expect(lines[0]).toBe("CONNECTED"); + expect(JSON.parse(lines[1] ?? "null")).toEqual({ + result: { + ok: false, + code: "ERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED", + message: expect.any(String), + }, + after: [{ x: 2 }], + }); + expect(exitCode).toBe(0); +} + +if (isDockerEnabled()) { + // CI: run against the docker-compose MySQL service. + describeWithContainer("mysql", { image: "mysql_plain" }, container => { + test("bind() does not OOB when the params array grows during binding", async () => { + await container.ready; + const url = `mysql://root@${container.host}:${container.port}/bun_sql_test`; + const { stdout, stderr, exitCode } = await runFixture(url); + assertFixtureOutput(stdout, stderr, exitCode); + }); + }); +} else { + // No docker daemon (e.g. local/sandboxed environments). If a MySQL server + // is reachable at MYSQL_URL or the conventional local address, exercise the + // fixture there so the regression is still covered. + const url = process.env.MYSQL_URL || "mysql://bun@127.0.0.1:3306/bun_sql_test"; + + describe("mysql (local)", () => { + test("bind() does not OOB when the params array grows during binding", async () => { + const { stdout, stderr, exitCode } = await runFixture(url); + // The fixture prints "CONNECTED" after the priming query succeeds. If + // it never got that far, there's no MySQL to talk to in this + // environment; the docker-gated branch above provides the CI coverage. + if (!stdout.startsWith("CONNECTED")) { + if (process.env.MYSQL_URL) { + // MYSQL_URL was explicitly provided; failing to connect is a real + // error, not an environment without MySQL. + throw new Error( + `sql-mysql-bind-oob: MYSQL_URL was provided but fixture never reached CONNECTED\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ); + } + console.warn("sql-mysql-bind-oob: no MySQL reachable at " + url + "; skipping assertions"); + return; + } + assertFixtureOutput(stdout, stderr, exitCode); + }); + }); +}