Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
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
14 changes: 13 additions & 1 deletion src/sql_jsc/mysql/JSMySQLQuery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,21 @@ impl JSMySQLQuery {
// R-2: errdefer rollback — `&Self` is `Copy`; the guard captures it by
// value, mutation is `JsCell`-backed, and `into_inner` disarms on the
// success path below.
//
// The guard only rolls back the `this_value` upgrade; it must NOT mark
// the query failed. Both callers already settle the promise on a run()
// error: `do_run` rethrows synchronously (the JS `.run()` frame rejects
// the promise), and the request-queue `advance()` loop calls
// `on_error`, which rejects via `reject_with_js_value`. Since that
// reject path itself transitions the query to `Fail` through
// `MySQLQuery::fail`, flipping the status here first would make
// `reject_with_js_value`'s `if !q.fail() { return }` guard bail out and
// silently drop the rejection — so a bind-time error on the
// prepare-then-execute path (e.g. a DATETIME whose year doesn't fit the
// wire `u16`, bound to a not-yet-prepared statement) would hang the
// promise forever instead of rejecting.
let errguard = scopeguard::guard(self, |s| {
s.this_value.with_mut(|v| v.downgrade());
let _ = s.query.with_mut(|q| q.fail());
});

let columns_value = self.get_columns().unwrap_or(JSValue::UNDEFINED);
Expand Down
102 changes: 46 additions & 56 deletions src/sql_jsc/mysql/MySQLValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,11 @@ impl DateTime {
)
}

pub fn from_unix_timestamp(timestamp: i64, microseconds: u32) -> DateTime {
pub fn from_unix_timestamp(
timestamp: i64,
microseconds: u32,
global_object: &JSGlobalObject,
) -> Result<DateTime, any_mysql_error::Error> {
let mut ts = timestamp;
let days = ts.div_euclid(86400);
ts = ts.rem_euclid(86400);
Expand All @@ -623,16 +627,31 @@ impl DateTime {
let minute = ts.div_euclid(60);
let second = ts.rem_euclid(60);

let date = gregorian_date(i32::try_from(days).expect("int cast"));
DateTime {
year: date.year,
// A numeric timestamp (the `is_number()` path in `from_js`) can carry a
// day count past `i32::MAX` without saturating `i64`. Route that cast
// through the same out-of-range error instead of panicking — the year
// check below rejects every in-`i32` value that still overflows `u16`.
let days = i32::try_from(days).map_err(|_| {
js_error_to_mysql(global_object.throw_invalid_arguments(format_args!(
"Date is out of range for a MySQL DATETIME",
)))
})?;
let date = gregorian_date(days);
let year = u16::try_from(date.year).map_err(|_| {
js_error_to_mysql(global_object.throw_invalid_arguments(format_args!(
"Date year {} is out of range for a MySQL DATETIME (0..=65535)",
date.year,
)))
})?;
Ok(DateTime {
year,
month: date.month,
day: date.day,
hour: u8::try_from(hour).expect("int cast"),
minute: u8::try_from(minute).expect("int cast"),
second: u8::try_from(second).expect("int cast"),
microsecond: microseconds,
}
})
}

pub fn to_js(self, global_object: &JSGlobalObject) -> JSValue {
Expand All @@ -643,22 +662,6 @@ impl DateTime {
)
}

/// `from_unix_timestamp`/`gregorian_date` can only represent
/// 1970-01-01T00:00:00Z through 9999-12-31T23:59:59Z (the MySQL DATETIME
/// maximum). Anything outside that window panics on an integer cast, so
/// reject it with a catchable error instead.
fn check_range(ts: i64, global_object: &JSGlobalObject) -> Result<(), any_mysql_error::Error> {
const MAX_DATETIME_UNIX_TIMESTAMP: i64 = 253_402_300_799;
if !(0..=MAX_DATETIME_UNIX_TIMESTAMP).contains(&ts) {
return Err(js_error_to_mysql(global_object.throw_invalid_arguments(
format_args!(
"MySQL DATE/DATETIME value must be between 1970-01-01T00:00:00Z and 9999-12-31T23:59:59Z"
),
)));
}
Ok(())
}

pub fn from_js(
value: JSValue,
global_object: &JSGlobalObject,
Expand All @@ -669,16 +672,14 @@ impl DateTime {
let total_ms = value.get_unix_timestamp();
let ts: i64 = (total_ms / 1000.0).floor() as i64;
let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32;
Self::check_range(ts, global_object)?;
return Ok(DateTime::from_unix_timestamp(ts, ms * 1000));
return DateTime::from_unix_timestamp(ts, ms * 1000, global_object);
}
Comment thread
robobun marked this conversation as resolved.

if value.is_number() {
let total_ms = value.as_number();
let ts: i64 = (total_ms / 1000.0).floor() as i64;
let ms: u32 = (total_ms - (ts as f64 * 1000.0)) as u32;
Self::check_range(ts, global_object)?;
return Ok(DateTime::from_unix_timestamp(ts, ms * 1000));
return DateTime::from_unix_timestamp(ts, ms * 1000, global_object);
Comment thread
robobun marked this conversation as resolved.
}

Err(js_error_to_mysql(global_object.throw_invalid_arguments(
Expand Down Expand Up @@ -871,45 +872,34 @@ impl Decimal {
// }
}

// Helper functions for date calculations
fn is_leap_year(year: u16) -> bool {
(year.is_multiple_of(4) && !year.is_multiple_of(100)) || year.is_multiple_of(400)
}

fn days_in_month(year: u16, month: u8) -> u8 {
const DAYS: [u8; 12] = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31];
if month == 2 && is_leap_year(year) {
return 29;
}
DAYS[month as usize - 1]
}

struct Date {
year: u16,
year: i64,
month: u8,
day: u8,
}

/// Civil year/month/day from a signed days-since-1970-01-01 count.
///
/// Uses Howard Hinnant's `civil_from_days` (400-year-era arithmetic) so
/// negative day counts — i.e. any pre-1970 `Date` parameter — yield the
/// correct calendar date instead of falling through loops that only walk
/// forwards from 1970. The proleptic year is returned unclamped; the caller
/// is responsible for rejecting years that don't fit the MySQL wire `u16`.
fn gregorian_date(days: i32) -> Date {
// Convert days since 1970-01-01 to year/month/day
let mut d = days;
let mut y: u16 = 1970;

while d >= 365 + is_leap_year(y) as i32 {
d -= 365 + is_leap_year(y) as i32;
y += 1;
}

let mut m: u8 = 1;
while d >= days_in_month(y, m) as i32 {
d -= days_in_month(y, m) as i32;
m += 1;
}
let z = i64::from(days) + 719468;
let era = z.div_euclid(146097);
let doe = (z - era * 146097) as u32; // [0, 146096]
let yoe = (doe - doe / 1460 + doe / 36524 - doe / 146096) / 365; // [0, 399]
let y = i64::from(yoe) + era * 400;
let doy = doe - (365 * yoe + yoe / 4 - yoe / 100); // [0, 365]
let mp = (5 * doy + 2) / 153; // [0, 11]
let d = doy - (153 * mp + 2) / 5 + 1; // [1, 31]
let m = if mp < 10 { mp + 3 } else { mp - 9 }; // [1, 12]

Date {
year: y,
month: m,
day: u8::try_from(d + 1).expect("int cast"),
year: y + i64::from(m <= 2),
month: m as u8,
day: d as u8,
}
}

Expand Down
126 changes: 126 additions & 0 deletions test/js/sql/sql-mysql-bind-error-hang.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// Regression: a bind-time encoder error on the *first* execution of a MySQL
// prepared statement used to hang the query's promise forever instead of
// rejecting it.
//
// Binding a Date whose year doesn't fit the DATETIME wire format (a `u16`)
// fails inside `Value::from_js`. When the statement hasn't been prepared yet,
// that error surfaces on the prepare-then-execute path through the request
// queue's `on_error` rather than synchronously from the `.run()` call.
// `JSMySQLQuery::run`'s error guard marked the query `Fail` before that async
// reject ran, so `reject_with_js_value`'s "already failed" guard bailed out and
// dropped the rejection — the awaited promise never settled.
//
// Exercised against a real MySQL server: the docker-compose `mysql_plain`
// service in CI, or a locally reachable server (MYSQL_URL / 127.0.0.1:3306)
// otherwise. A year beyond the `u16` range (70000) is out of range for every
// MySQL version, so no server-side DATETIME support is required.

import { SQL } from "bun";
import { describe, expect, test } from "bun:test";
import { describeWithContainer, isDockerEnabled } from "harness";

// A statement whose first execution binds an out-of-range DATETIME parameter.
// No priming query first, so this is the statement's first use and the error
// travels the async prepare-then-execute path the fix repairs.
async function runBindErrorHang(sql: SQL) {
const farFuture = new Date("+070000-01-01T00:00:00.000Z");
expect(farFuture.getUTCFullYear()).toBe(70000);

// A Bun SQL query is a single-consumption thenable, so await it inside a
// wrapper promise rather than handing the query object straight to
// `expect().rejects` (which `.then()`s it more than once and would hang).
// Before the fix this rejection never arrived at all — the promise hung on
// the prepare-then-execute path.
await expect(
(async () => {
await sql`SELECT ${farFuture} AS dt`;
})(),
).rejects.toThrow(/year 70000 is out of range/i);

// The connection must stay usable after the rejected bind.
expect((await sql`SELECT 1 AS ok`)[0].ok).toBe(1);
}

// `Signature::generate` and `bind` each iterate the user's param array, so an
// index getter can hand a `Date` to the first pass (making the column a
// DATETIME) and a number to the second. A huge number yields a day count past
// `i32::MAX`; the encoder's `i32::try_from(days)` used to `.expect()`-panic
// (process abort) on that value instead of rejecting.
async function runGetterMutationAbort(sql: SQL) {
// Prime the prepared-statement cache with a DATETIME signature.
await sql.unsafe("select ? as d", [new Date(0)]);

let reads = 0;
const values: unknown[] = [new Date("2020-01-01T00:00:00.000Z")];
Object.defineProperty(values, "0", {
enumerable: true,
configurable: true,
get() {
reads++;
// First pass (signature): a Date -> column bound as DATETIME.
// Later pass (bind): a number whose day count overflows i32.
return reads <= 1 ? new Date("2020-01-01T00:00:00.000Z") : 1e20;
},
});

const result = await sql.unsafe("select ? as d", values).then(
rows => ({ ok: true, rows }),
(err: any) => ({ ok: false, code: err?.code, message: String(err?.message ?? err) }),
);
expect(result).toMatchObject({ ok: false, code: "ERR_INVALID_ARG_TYPE" });
expect(reads).toBeGreaterThanOrEqual(2);

// The connection must still be usable after the rejected bind.
expect((await sql.unsafe("select ? as x", [2]))[0].x).toBe(2);
}

if (isDockerEnabled()) {
describeWithContainer("mysql", { image: "mysql_plain" }, container => {
const getUrl = () => `mysql://root@${container.host}:${container.port}/bun_sql_test`;
test("a bind error on a statement's first use rejects instead of hanging", async () => {
await container.ready;
await using sql = new SQL({ url: getUrl(), max: 1 });
await runBindErrorHang(sql);
});
test("an out-of-range DATETIME from an array-index getter rejects instead of aborting", async () => {
await container.ready;
await using sql = new SQL({ url: getUrl(), max: 1 });
await runGetterMutationAbort(sql);
});
});
} 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 fix
// there; the docker-gated branch above provides the CI coverage.
const url = process.env.MYSQL_URL || "mysql://bun@127.0.0.1:3306/bun_sql_test";

// Returns a connected SQL client, or null if no MySQL is reachable (and
// MYSQL_URL was not explicitly provided, in which case it's a soft skip).
async function connectOrSkip(sql: SQL, label: string): Promise<boolean> {

Check warning on line 99 in test/js/sql/sql-mysql-bind-error-hang.test.ts

View check run for this annotation

Claude / Claude Code Review

Stale doc comment on connectOrSkip

The comment above `connectOrSkip` says "Returns a connected SQL client, or null if no MySQL is reachable", but the function takes the `SQL` client as a parameter and returns `Promise<boolean>` (true on connect, false on soft-skip). Looks like the comment was left over from an earlier iteration before the helper was refactored — just needs updating to match the boolean return.
Comment thread
robobun marked this conversation as resolved.
Outdated
try {
await sql`SELECT 1`;
return true;
} catch (e) {
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(`${label}: MYSQL_URL was provided but the server is unreachable: ${e}`);
}
console.warn(`${label}: no MySQL reachable at ${url}; skipping assertions`);
return false;
}
}

describe("mysql (local)", () => {
test("a bind error on a statement's first use rejects instead of hanging", async () => {
await using sql = new SQL({ url, max: 1 });
if (!(await connectOrSkip(sql, "sql-mysql-bind-error-hang"))) return;
await runBindErrorHang(sql);
});
test("an out-of-range DATETIME from an array-index getter rejects instead of aborting", async () => {
await using sql = new SQL({ url, max: 1 });
if (!(await connectOrSkip(sql, "sql-mysql-bind-error-hang"))) return;
await runGetterMutationAbort(sql);
});
});
}
69 changes: 69 additions & 0 deletions test/js/sql/sql-mysql.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
// The DATETIME encoder writes UTC y/m/d fields but the decoder currently
// reconstructs a local-time Date, so the pre-1970 round-trip test below only
// matches when local == UTC. Pin TZ before any imports so the runtime picks
// it up. This can be dropped once the decoder is moved to UTC.
process.env.TZ = "Etc/UTC";

import { SQL, randomUUIDv7 } from "bun";
import { beforeAll, describe, expect, mock, test } from "bun:test";
import { bunEnv, bunRun, describeWithContainer, isDockerEnabled, tempDirWithFiles } from "harness";
Expand Down Expand Up @@ -116,6 +122,69 @@ if (isDockerEnabled()) {
expect(rawRow[2]).toEqual(new Uint8Array([0xce, 0xff, 0xff])); // -50 as i24 LE
expect(Buffer.from(rawRow[5]).toString("utf-8")).toBe("alice");
});
test("pre-1970 Date parameters round-trip through DATETIME", async () => {
// The day-count -> civil-date conversion only walked forwards from
// 1970, so a negative day count fell through to a negative `day`
// and `u8::try_from(negative).expect()` aborted while encoding the
// bound parameter.
await using db = new SQL({ ...getOptions(), max: 1, idleTimeout: 5 });
using sql = await db.reserve();
const t = "dt_" + randomUUIDv7("hex").replaceAll("-", "");
await sql`CREATE TEMPORARY TABLE ${sql(t)} (id INT PRIMARY KEY, dt DATETIME)`;
const cases = [
new Date("1969-07-20T20:17:40.000Z"),
new Date("1969-12-31T00:00:00.000Z"),
new Date("1900-01-01T00:00:00.000Z"),
new Date("1970-01-01T00:00:00.000Z"),
new Date("2024-02-29T12:34:56.000Z"),
];
for (const [id, dt] of cases.entries()) {
await sql`INSERT INTO ${sql(t)} VALUES (${id}, ${dt})`;
}
const rows = await sql`SELECT id, dt FROM ${sql(t)} ORDER BY id`;
expect(rows).toEqual(cases.map((dt, id) => ({ id, dt })));
});
test("Date with year outside the MySQL u16 range is rejected, not silently clamped", async () => {
await using db = new SQL({ ...getOptions(), max: 1, idleTimeout: 5 });
using sql = await db.reserve();
const t = "yr_" + randomUUIDv7("hex").replaceAll("-", "");
await sql`CREATE TEMPORARY TABLE ${sql(t)} (id INT PRIMARY KEY, dt DATETIME)`;
// First insert primes the prepared-statement cache so the next
// execution binds synchronously and the encoder error surfaces
// directly as a rejection.
await sql`INSERT INTO ${sql(t)} VALUES (${0}, ${new Date(0)})`;
const farFuture = new Date("+070000-01-01T00:00:00.000Z");
expect(farFuture.getUTCFullYear()).toBe(70000);
// A Bun SQL query is a single-consumption thenable; await it inside a
// wrapper promise rather than handing the query object to
// `expect().rejects` (which `.then()`s it twice and would hang).
await expect(
(async () => {
await sql`INSERT INTO ${sql(t)} VALUES (${1}, ${farFuture})`;
})(),
).rejects.toThrow(/year 70000 is out of range/i);
});
test("a bind error on the first use of a statement rejects instead of hanging", async () => {
// When the out-of-range Date is bound to a statement that hasn't been
// prepared yet, the error surfaces on the prepare-then-execute path
// through the request queue's `on_error` rather than synchronously.
// `run()`'s error guard used to mark the query failed before that
// reject ran, so `reject_with_js_value`'s "already failed" guard
// dropped the rejection and the promise hung forever. No priming
// query here: this is the statement's first execution.
await using db = new SQL({ ...getOptions(), max: 1, idleTimeout: 10 });
using sql = await db.reserve();
const t = "hang_" + randomUUIDv7("hex").replaceAll("-", "");
await sql`CREATE TEMPORARY TABLE ${sql(t)} (id INT PRIMARY KEY, dt DATETIME)`;
const farFuture = new Date("+070000-01-01T00:00:00.000Z");
await expect(
(async () => {
await sql`INSERT INTO ${sql(t)} VALUES (${1}, ${farFuture})`;
})(),
).rejects.toThrow(/year 70000 is out of range/i);
// The connection is still usable after the rejected bind.
expect((await sql`SELECT 1 AS ok`)[0].ok).toBe(1);
});
describe("should work with more than the max inline capacity", () => {
for (let size of [50, 60, 62, 64, 70, 100]) {
for (let duplicated of [true, false]) {
Expand Down
Loading