From dfd2a3cfcc0183da10167ff45565e16d27f69a2b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 28 Jun 2026 17:36:23 +0000 Subject: [PATCH] Stop dispatching Postgres messages once the connection has failed A backend message that fails the connection can share a TCP read with messages that follow it. PostgresRequest::on_data's message loop kept dispatching those trailing messages against the already-failed connection. A ReadyForQuery in that position called set_status(Status::Connected), which had no guard against leaving Failed, so the dead connection was flipped back to Connected and its idle timer was re-armed on a socket uSockets had already scheduled to free. When that timer later fired, ref_and_close read the freed us_socket_t: AddressSanitizer: heap-use-after-free in us_socket_is_closed PostgresSQLConnection::ref_and_close PostgresSQLConnection::fail_with_js_value PostgresSQLConnection::on_connection_timeout The message loop now returns once the status is Failed, and set_status refuses to transition out of Failed. --- src/sql_jsc/postgres/PostgresRequest.rs | 7 +++ src/sql_jsc/postgres/PostgresSQLConnection.rs | 8 +++- ...-failed-connection-resurrection.fixture.ts | 43 +++++++++++++++++ ...res-failed-connection-resurrection.test.ts | 47 +++++++++++++++++++ 4 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 test/js/sql/postgres-failed-connection-resurrection.fixture.ts create mode 100644 test/js/sql/postgres-failed-connection-resurrection.test.ts diff --git a/src/sql_jsc/postgres/PostgresRequest.rs b/src/sql_jsc/postgres/PostgresRequest.rs index d0257199a033..72ecc08dbbd4 100644 --- a/src/sql_jsc/postgres/PostgresRequest.rs +++ b/src/sql_jsc/postgres/PostgresRequest.rs @@ -5,6 +5,7 @@ use bun_core::fmt as bun_fmt; use bun_sql::postgres::PostgresProtocol as protocol; use bun_sql::postgres::PostgresTypes as types; use bun_sql::postgres::PostgresTypes::{AnyPostgresError, Int4, Short}; +use bun_sql::postgres::Status; use bun_sql::postgres::protocol::{ReaderContext, WriterContext}; use crate::jsc::js_error_to_postgres; @@ -437,6 +438,12 @@ pub(crate) fn on_data( ) -> Result<(), AnyPostgresError> { use MessageType as M; loop { + // `fail()` inside a handler tears the connection down (status = Failed, + // socket closed, queue rejected). Stop dispatching: later messages in + // the same read must not act on the dead connection. + if connection.status.get() == Status::Failed { + return Ok(()); + } reader.mark_message_start(); let c = reader.int::()?; bun_core::scoped_log!(Postgres, "read: {}", c as char); diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index a99b5da8861e..53bd07095ca8 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -616,7 +616,13 @@ impl PostgresSQLConnection { } pub fn set_status(&self, status: Status) { - if self.status.get() == status { + let current = self.status.get(); + if current == status { + return; + } + // `Failed` is terminal: `fail_with_js_value` already closed the socket + // and rejected every pending request. Nothing may transition out of it. + if current == Status::Failed { return; } // reshaped for borrowck — `defer this.updateHasPendingActivity()` moved to explicit calls below. diff --git a/test/js/sql/postgres-failed-connection-resurrection.fixture.ts b/test/js/sql/postgres-failed-connection-resurrection.fixture.ts new file mode 100644 index 000000000000..f1e7f1c6fdc0 --- /dev/null +++ b/test/js/sql/postgres-failed-connection-resurrection.fixture.ts @@ -0,0 +1,43 @@ +// Fixture for postgres-failed-connection-resurrection.test.ts. Run as a +// subprocess so that an AddressSanitizer abort is observable as a non-zero +// exit code from the test. +// +// Mock backend that answers the StartupMessage with ONE write carrying TWO +// backend messages: an Authentication request with an unrecognized type (99), +// immediately followed by ReadyForQuery. The unrecognized type fails the +// connection mid-read; the trailing ReadyForQuery from the same read must not +// be dispatched against the now-dead connection. +import { SQL } from "bun"; +import { listeningServer, pgInt32, pgRaw, pgReadyForQuery } from "./wire-frames"; + +const { port, server } = await listeningServer(socket => { + socket.once("data", () => { + socket.write(Buffer.concat([pgRaw("R", pgInt32(99)), pgReadyForQuery()])); + }); + socket.on("error", () => {}); +}); + +const sql = new SQL({ + url: `postgres://postgres@127.0.0.1:${port}/postgres`, + max: 1, + idleTimeout: 1, + connectionTimeout: 5, +}); + +try { + await sql`select 1`; + console.log("RESOLVED"); +} catch (err: any) { + console.log(err?.code ?? String(err)); +} + +// Before the fix, the ReadyForQuery flipped the failed connection back to +// Connected and re-armed its 1s idle timer on a socket uSockets had already +// scheduled to free; the timer callback then dereferenced the freed socket. +// Keep the process alive past that window. There is no event to await: a +// correct build simply never fires that timer again. +await Bun.sleep(1600); +console.log("SURVIVED"); + +await sql.close({ timeout: 0 }); +await new Promise(resolve => server.close(() => resolve())); diff --git a/test/js/sql/postgres-failed-connection-resurrection.test.ts b/test/js/sql/postgres-failed-connection-resurrection.test.ts new file mode 100644 index 000000000000..aed78cf8c01a --- /dev/null +++ b/test/js/sql/postgres-failed-connection-resurrection.test.ts @@ -0,0 +1,47 @@ +// Fault-injection test: requires a server that refuses / drops / sends malformed +// frames, which a healthy container will not do on demand. DO NOT COPY THIS +// PATTERN — anything a real server can produce belongs in describeWithContainer. +// All wire-protocol bytes come from test/js/sql/wire-frames.ts; do not inline +// Buffer.alloc frame construction here. +// +// A backend message that fails the connection can share a read with messages +// that follow it. The message loop used to keep dispatching those trailing +// messages against the already-failed connection; a ReadyForQuery in that +// position flipped the status back to Connected and re-armed the idle timer +// on a socket uSockets had already scheduled to free, so the timer callback +// later read freed memory: +// AddressSanitizer: heap-use-after-free in us_socket_is_closed +// PostgresSQLConnection::ref_and_close <- fail_with_js_value +// <- fail_fmt <- on_connection_timeout +// The read of freed memory is only detectable under ASan, so this is gated to +// ASan builds; release lanes would pass regardless of the bug. +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isASAN } from "harness"; +import path from "node:path"; + +test.skipIf(!isASAN)( + "a failed connection is not resurrected by trailing messages in the same read", + async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "postgres-failed-connection-resurrection.fixture.ts")], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + timeout: 25_000, + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ + stdout: stdout.trim().split(/\r?\n/), + exitCode, + // not asserted (ASan/debug builds emit benign notes); included so the + // ASan report shows up in the diff when the fixture dies + stderr, + }).toEqual({ + stdout: ["ERR_POSTGRES_UNKNOWN_AUTHENTICATION_METHOD", "SURVIVED"], + exitCode: 0, + stderr: expect.any(String), + }); + }, + // the fixture is a debug+ASan bun that intentionally outlives a 1s timer + 30_000, +);