diff --git a/src/js/bun/sql.ts b/src/js/bun/sql.ts index c3a74582a259..93c36fd36e05 100644 --- a/src/js/bun/sql.ts +++ b/src/js/bun/sql.ts @@ -7,7 +7,7 @@ const { Query, SQLQueryFlags } = require("internal/sql/query"); const { PostgresAdapter } = require("internal/sql/postgres"); const { MySQLAdapter } = require("internal/sql/mysql"); const { SQLiteAdapter } = require("internal/sql/sqlite"); -const { SQLHelper, parseOptions } = require("internal/sql/shared"); +const { SQLHelper, parseOptions, validateCloseTimeout } = require("internal/sql/shared"); const { SQLError, PostgresError, SQLiteError, MySQLError } = require("internal/sql/errors"); @@ -386,13 +386,12 @@ const SQL: typeof Bun.SQL = function SQL( ) { return Promise.$resolve(undefined); } - state.connectionState &= ~ReservedConnectionState.acceptQueries; let timeout = options?.timeout; if (timeout) { - timeout = Number(timeout); - if (timeout > 2 ** 31 || timeout < 0 || timeout !== timeout) { - throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); - } + timeout = validateCloseTimeout(timeout); + } + state.connectionState &= ~ReservedConnectionState.acceptQueries; + if (timeout) { if (timeout > 0 && (reserveQueries.size > 0 || reservedTransaction.size > 0)) { const { promise, resolve } = Promise.withResolvers(); // race all queries vs timeout @@ -651,15 +650,13 @@ const SQL: typeof Bun.SQL = function SQL( ) { return Promise.$resolve(undefined); } + let timeout = options?.timeout; + if (timeout) { + timeout = validateCloseTimeout(timeout); + } state.connectionState &= ~ReservedConnectionState.acceptQueries; const transactionQueries = state.queries; - let timeout = options?.timeout; if (timeout) { - timeout = Number(timeout); - if (timeout > 2 ** 31 || timeout < 0 || timeout !== timeout) { - throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); - } - if (timeout > 0 && (transactionQueries.size > 0 || transactionSavepoints.size > 0)) { const { promise, resolve } = Promise.withResolvers(); // race all queries vs timeout diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts index 94e82acef855..669e8dd5e56a 100644 --- a/src/js/internal/sql/shared.ts +++ b/src/js/internal/sql/shared.ts @@ -147,6 +147,23 @@ function normalizeSSLMode(value: string): SSLMode { throw $ERR_INVALID_ARG_VALUE("sslmode", value, "must be one of: disable, prefer, require, verify-ca, verify-full"); } +// close({ timeout }) takes seconds but arms its deadline timer in +// milliseconds, and setTimeout clamps delays above 2 ** 31 - 1 ms to 1 ms, so +// the upper bound must hold for the millisecond product. +const MAX_CLOSE_TIMEOUT = (2 ** 31 - 1) / 1000; + +function validateCloseTimeout(timeout: number): number { + timeout = Number(timeout); + if (timeout > MAX_CLOSE_TIMEOUT || timeout < 0 || timeout !== timeout) { + throw $ERR_INVALID_ARG_VALUE( + "options.timeout", + timeout, + `must be a non-negative number no greater than ${MAX_CLOSE_TIMEOUT} seconds`, + ); + } + return timeout; +} + export type { SQLHelper }; class SQLHelper { public readonly value: T; @@ -1204,10 +1221,7 @@ abstract class BaseSQLAdapter 2 ** 31 || timeout < 0 || timeout !== timeout) { - throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); - } + timeout = validateCloseTimeout(timeout); this.closed = true; if (timeout === 0 || !this.hasPendingQueries()) { @@ -2119,6 +2133,7 @@ export default { SQLHelper, buildDefinedColumnsAndQuery, normalizeSSLMode, + validateCloseTimeout, SQLResultArray, SQLArrayParameter, getHelperCommandFromDetect, diff --git a/test/js/sql/sql-close-timeout-overflow.test.ts b/test/js/sql/sql-close-timeout-overflow.test.ts new file mode 100644 index 000000000000..5807d0b66074 --- /dev/null +++ b/test/js/sql/sql-close-timeout-overflow.test.ts @@ -0,0 +1,115 @@ +// close({ timeout }) takes seconds but arms its deadline timer in +// milliseconds. The upper bound used to be checked against the seconds value +// (2 ** 31), so timeouts in (2147483.647, 2 ** 31] seconds passed validation, +// overflowed setTimeout's 32-bit millisecond range (which clamps to 1 ms) and +// force-closed after ~1 ms, cancelling in-flight queries. On reserved +// connections and transactions, validation also ran after acceptQueries was +// already cleared, so a rejected close left the handle refusing all further +// queries. +// https://github.com/oven-sh/bun/issues/32096 + +import { SQL } from "bun"; +import { expect, test } from "bun:test"; +import { describeWithContainer, isDockerEnabled } from "harness"; + +// setTimeout's maximum delay is 2 ** 31 - 1 milliseconds. +const MAX_TIMEOUT_SECONDS = (2 ** 31 - 1) / 1000; // 2147483.647 + +// Pool connections are created lazily and close() validates its options +// before touching any connection, so no database server is needed here. +const adapters = [ + ["postgres", "postgres://bun_sql_test@localhost:5432/bun_sql_test"], + ["mysql", "mysql://bun_sql_test@localhost:3306/bun_sql_test"], +] as const; + +for (const [adapter, url] of adapters) { + test.each([2 ** 31, 2147483.648, Infinity])( + `${adapter}: close() rejects timeout %p, whose millisecond value overflows setTimeout`, + async timeout => { + await using sql = new SQL(url, { max: 1 }); + const err = await sql.close({ timeout }).then( + () => null, + e => e, + ); + expect(err?.code).toBe("ERR_INVALID_ARG_VALUE"); + }, + ); + + test.each([MAX_TIMEOUT_SECONDS, 60, 0.5])(`${adapter}: close() accepts timeout %p`, async timeout => { + await using sql = new SQL(url, { max: 1 }); + await sql.close({ timeout }); + }); + + // Values the old bound already rejected must stay rejected. + test.each([-1, "not a number"])(`${adapter}: close() still rejects timeout %p`, async timeout => { + await using sql = new SQL(url, { max: 1 }); + const err = await sql.close({ timeout: timeout as number }).then( + () => null, + e => e, + ); + expect(err?.code).toBe("ERR_INVALID_ARG_VALUE"); + }); +} + +test("close() rejection names the real bound", async () => { + await using sql = new SQL("postgres://bun_sql_test@localhost:5432/bun_sql_test", { max: 1 }); + const err = await sql.close({ timeout: 2 ** 31 }).then( + () => null, + e => e, + ); + expect(err?.message).toBe( + "The property 'options.timeout' must be a non-negative number no greater than 2147483.647 seconds. Received 2147483648", + ); +}); + +// transaction_sql.close() shares the same validation. SQLite transactions run +// in-process, so this exercises it without a server. Before the fix the +// overflowing timeout was accepted: the transaction was rolled back, queries +// after the close rejected with ERR_SQLITE_CONNECTION_CLOSED, and the commit +// rejected the begin() promise. +test("sqlite: transaction close() rejects an overflowing timeout and leaves the transaction usable", async () => { + await using sql = new SQL("sqlite://:memory:"); + let closeError: any = null; + let rowsAfterRejectedClose: any = null; + const result = await sql + .begin(async tx => { + closeError = await tx.close({ timeout: 2 ** 31 }).then( + () => null, + e => e, + ); + rowsAfterRejectedClose = await tx`select 1 as x`.then( + rows => rows[0], + e => `query failed: ${e.code}`, + ); + return "callback completed"; + }) + .then( + value => value, + e => `begin rejected: ${e.code}`, + ); + expect(closeError?.code).toBe("ERR_INVALID_ARG_VALUE"); + expect(rowsAfterRejectedClose).toEqual({ x: 1 }); + expect(result).toBe("callback completed"); +}); + +// reserved_sql.close() is the remaining variant; it needs a real connection, +// so it runs against the docker postgres service when available. +if (isDockerEnabled()) { + describeWithContainer("postgres", { image: "postgres_plain" }, container => { + test("reserved connection close() rejects an overflowing timeout and stays usable", async () => { + await container.ready; + const url = `postgres://bun_sql_test@${container.host}:${container.port}/bun_sql_test`; + await using sql = new SQL(url, { max: 2 }); + // disposal calls release(); it runs before the pool's own disposal + await using reserved = await sql.reserve(); + const err = await reserved.close({ timeout: 2 ** 31 }).then( + () => null, + e => e, + ); + expect(err?.code).toBe("ERR_INVALID_ARG_VALUE"); + // the rejected close must leave the reserved connection untouched + const rows = await reserved`select 1 as x`; + expect(rows[0]).toEqual({ x: 1 }); + }); + }); +}