Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 7 additions & 0 deletions src/sql_jsc/postgres/PostgresRequest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -437,6 +438,12 @@ pub(crate) fn on_data<Context: ReaderContext>(
) -> 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::<u8>()?;
bun_core::scoped_log!(Postgres, "read: {}", c as char);
Expand Down
8 changes: 7 additions & 1 deletion src/sql_jsc/postgres/PostgresSQLConnection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
43 changes: 43 additions & 0 deletions test/js/sql/postgres-failed-connection-resurrection.fixture.ts
Original file line number Diff line number Diff line change
@@ -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<void>(resolve => server.close(() => resolve()));
47 changes: 47 additions & 0 deletions test/js/sql/postgres-failed-connection-resurrection.test.ts
Original file line number Diff line number Diff line change
@@ -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,
);
Loading