Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
125 changes: 69 additions & 56 deletions src/sql_jsc/mysql/MySQLValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,22 @@
}
}

/// Rejects a non-finite millisecond timestamp (`NaN`/±Inf) before it reaches
/// the float->int casts that would silently turn it into 0. An Invalid Date
/// (`new Date(NaN)`) is still a `Date`, so it would otherwise be bound as a
/// plausible-looking `1970-01-01 00:00:00` instead of surfacing the error.
fn check_finite_ms(
total_ms: f64,
global_object: &JSGlobalObject,
) -> Result<(), any_mysql_error::Error> {
if !total_ms.is_finite() {
return Err(js_error_to_mysql(global_object.throw_invalid_arguments(
format_args!("Invalid Date cannot be bound as a MySQL DATE/DATETIME/TIME value"),
)));
}
Ok(())
}

#[derive(Default, Clone, Copy)]
pub struct DateTime {
pub year: u16,
Expand Down Expand Up @@ -612,7 +628,11 @@
)
}

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 +643,31 @@
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 +678,6 @@
)
}

/// `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 @@ -667,18 +686,21 @@
if value.is_date() {
// this is actually ms not seconds
let total_ms = value.get_unix_timestamp();
// An Invalid Date (`new Date(NaN)`) carries NaN; the float->int
// casts below would silently launder it into 0 (1970-01-01 on the
// wire). Reject it instead of storing a bogus timestamp.
check_finite_ms(total_ms, global_object)?;
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();
check_finite_ms(total_ms, global_object)?;
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);

Check warning on line 703 in src/sql_jsc/mysql/MySQLValue.rs

View check run for this annotation

Claude / Claude Code Review

ms * 1000 u32 overflow evaluated before from_unix_timestamp's range guard (debug-build abort)

`ms * 1000` is evaluated as a call argument *before* `from_unix_timestamp`'s `i32::try_from(days)` guard runs, so a numeric timestamp like `1e22` (which saturates `ts` to `i64::MAX` and `ms` to `u32::MAX`) overflows `u32` and aborts a debug build — pre-PR, `check_range(ts)?` ran first and rejected it. Release builds wrap and are then caught by the `days` guard, so this is debug-only on the same adversarial getter-mutation path `runGetterMutationAbort` already covers; but since this PR is specifi
Comment thread
robobun marked this conversation as resolved.
}

Err(js_error_to_mysql(global_object.throw_invalid_arguments(
Expand Down Expand Up @@ -718,12 +740,14 @@
// TODO(port): narrow error set
if value.is_date() {
let total_ms = value.get_unix_timestamp();
check_finite_ms(total_ms, global_object)?;
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)?;
Ok(Time::from_unix_timestamp(ts, ms * 1000))
} else if value.is_number() {
let total_ms = value.as_number();
check_finite_ms(total_ms, global_object)?;
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)?;
Expand Down Expand Up @@ -871,45 +895,34 @@
// }
}

// 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
155 changes: 155 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,155 @@
// 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);
}

// An Invalid Date (`new Date(NaN)`) is still a `Date`, so it binds as a
// DATETIME. Its timestamp is NaN, which the float->int cast would launder into
// 0 and silently store as 1970-01-01. It must be rejected instead.
async function runInvalidDateReject(sql: SQL) {
for (const invalid of [new Date("not a date"), new Date(NaN), new Date(Infinity)]) {
expect(Number.isNaN(invalid.getTime())).toBe(true);
await expect(
(async () => {
await sql`SELECT ${invalid} AS dt`;
})(),
).rejects.toThrow(/invalid date/i);
}

// The connection must stay usable after the rejected binds.
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);
});
test("an Invalid Date bound as DATETIME is rejected, not stored as 1970-01-01", async () => {
await container.ready;
await using sql = new SQL({ url: getUrl(), max: 1 });
await runInvalidDateReject(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";

// Probes the connection with a trivial query. Returns true when the server
// is reachable. Returns false (after a warning) for a soft skip when no MySQL
// is reachable and MYSQL_URL was not explicitly provided; throws if MYSQL_URL
// was provided but the server is unreachable.
async function connectOrSkip(sql: SQL, label: string): Promise<boolean> {
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);
});
test("an Invalid Date bound as DATETIME is rejected, not stored as 1970-01-01", async () => {
await using sql = new SQL({ url, max: 1 });
if (!(await connectOrSkip(sql, "sql-mysql-bind-error-hang"))) return;
await runInvalidDateReject(sql);
});
});
}
Loading