sql(mysql): bounds-check params in bind() when values array is mutated

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;
 }
 

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();
+}

test/js/sql/sql-mysql-bind-oob.test.ts

@@ -0,0 +1,75 @@
+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 (and sql-mysql.test.ts)
+      // provide the CI coverage.
+      if (!stdout.startsWith("CONNECTED")) {
+        console.warn("sql-mysql-bind-oob: no MySQL reachable at " + url + "; skipping assertions");
+        return;
+      }
+      assertFixtureOutput(stdout, stderr, exitCode);
+    }, 30_000);
+  });
+}