From ec8050c8c1f78d609ae8ef0b5127db8c0399c2cd Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 11 Jun 2026 06:10:09 +0000 Subject: [PATCH 1/2] sql: reject close() timeouts that overflow setTimeout's millisecond range The timeout option is in seconds but the deadline timer is armed in milliseconds. The upper bound was checked against the seconds value (2 ** 31), so timeouts in (2147483.647, 2 ** 31] passed validation and setTimeout clamped the overflowing delay to 1 ms, force-closing the pool and cancelling in-flight queries. All four close() sites now share one validator bounded at (2 ** 31 - 1) / 1000 seconds. On reserved connections and transactions, validation now runs before acceptQueries is cleared so a rejected close leaves the handle usable. Fixes #32096 --- src/js/bun/sql.ts | 21 ++-- src/js/internal/sql/shared.ts | 23 +++- .../js/sql/sql-close-timeout-overflow.test.ts | 115 ++++++++++++++++++ 3 files changed, 143 insertions(+), 16 deletions(-) create mode 100644 test/js/sql/sql-close-timeout-overflow.test.ts 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..00a519674ca2 --- /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 }); + const 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 }); + reserved.release(); + }); + }); +} From 7fadfe20bdfba98da9d44669d5b45366bc7050d5 Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 11 Jun 2026 06:18:43 +0000 Subject: [PATCH 2/2] test: register reserved connection cleanup via await using --- test/js/sql/sql-close-timeout-overflow.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/js/sql/sql-close-timeout-overflow.test.ts b/test/js/sql/sql-close-timeout-overflow.test.ts index 00a519674ca2..5807d0b66074 100644 --- a/test/js/sql/sql-close-timeout-overflow.test.ts +++ b/test/js/sql/sql-close-timeout-overflow.test.ts @@ -100,7 +100,8 @@ if (isDockerEnabled()) { await container.ready; const url = `postgres://bun_sql_test@${container.host}:${container.port}/bun_sql_test`; await using sql = new SQL(url, { max: 2 }); - const reserved = await sql.reserve(); + // 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, @@ -109,7 +110,6 @@ if (isDockerEnabled()) { // the rejected close must leave the reserved connection untouched const rows = await reserved`select 1 as x`; expect(rows[0]).toEqual({ x: 1 }); - reserved.release(); }); }); }