From b40d131078e5f8be23105ee3ec4d7481019aee51 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 27 Jun 2026 16:30:27 +0000 Subject: [PATCH 1/4] sql: detach the stored socket in on_close/on_connect_error PostgresSQLConnection and JSMySQLConnection kept the raw us_socket_t pointer in self.socket after the on_close/on_connect_error dispatch. usockets frees closed sockets at us_internal_loop_post later in the same tick, so any subsequent read of self.socket (the connectionTimeout EventLoopTimer, ref()/unref()/close() on the native handle) dereferenced freed memory. Detach the stored handle at the start of on_close and on_connect_error, matching what the Valkey client already does. --- src/sql_jsc/mysql/JSMySQLConnection.rs | 9 + src/sql_jsc/postgres/PostgresSQLConnection.rs | 9 + test/js/sql/sql-connection-socket-uaf.test.ts | 165 ++++++++++++++++++ 3 files changed, 183 insertions(+) create mode 100644 test/js/sql/sql-connection-socket-uaf.test.ts diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index d349bee95f56..62c4db64fe4d 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -1008,6 +1008,11 @@ impl SocketHandler { // RAII guard adopts that existing ref (no `ref_()` here); raw-pointer // shaped so no reference outlives the potential free. let _ref = DerefOnDrop(this.as_ctx_ptr()); + // usockets frees this socket at end-of-tick; drop the stored pointer + // now so nothing (timer callbacks, do_close) dereferences it after + // the free. + this.connection_mut() + .set_socket(AnySocket::SocketTcp(SocketTCP::detached())); // A close before the handshake finished means the server (or an // intermediary like a container port proxy) accepted the TCP // connection but went away before completing startup — e.g. the @@ -1031,6 +1036,10 @@ impl SocketHandler { } pub fn on_connect_error(this: &JSMySQLConnection, _: NewSocketHandler, _: i32) { + // The dispatch trampoline already closed the connecting socket; it is + // freed at end-of-tick, so detach before any user-visible callback. + this.connection_mut() + .set_socket(AnySocket::SocketTcp(SocketTCP::detached())); this.fail(b"Failed to connect", AnyMySQLErrorT::ConnectionRefused); } diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index a99b5da8861e..676f23e9ff6e 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -1343,6 +1343,11 @@ impl SocketHandler { _: i32, _: Option<*mut c_void>, ) { + // usockets frees this socket at end-of-tick; drop the stored pointer + // now so nothing (timer callbacks, ref()/unref()) dereferences it + // after the free. + this.socket + .set(Socket::SocketTcp(uws::SocketTCP::detached())); this.on_close(); } @@ -1351,6 +1356,10 @@ impl SocketHandler { } pub fn on_connect_error(this: &PostgresSQLConnection, _socket: SocketType, _: i32) { + // The dispatch trampoline already closed the connecting socket; it is + // freed at end-of-tick, so detach before any user-visible callback. + this.socket + .set(Socket::SocketTcp(uws::SocketTCP::detached())); Self::guarded(this, |t| t.on_connect_error()); } diff --git a/test/js/sql/sql-connection-socket-uaf.test.ts b/test/js/sql/sql-connection-socket-uaf.test.ts new file mode 100644 index 000000000000..10129f3c84a0 --- /dev/null +++ b/test/js/sql/sql-connection-socket-uaf.test.ts @@ -0,0 +1,165 @@ +// 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. +// +// PostgresSQLConnection/MySQLConnection kept the raw us_socket_t* in +// self.socket after usockets dispatched on_close/on_connect_error. usockets +// frees closed sockets at the end of the same tick, so any later read of +// self.socket.is_closed() (connection.ref()/.unref()/.close(), or the +// connectionTimeout timer) was a heap-use-after-free. Valkey already +// detaches its stored socket in the same callbacks; Postgres and MySQL now +// do too. +// +// The fixture captures the native connection by shadowing the query +// handle's .run(connection, query), lets the server drop the socket, yields +// past us_internal_free_closed_sockets, then touches the native connection +// through a method that reads the stored socket pointer. + +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isASAN, isDebug, tempDir } from "harness"; +import path from "node:path"; + +const wireFrames = path.join(import.meta.dir, "wire-frames.ts"); + +const drivers = [ + { + name: "postgres", + // .ref() calls update_has_pending_activity() which reads + // self.socket.is_closed() when the connection is in a terminal state. + touch: "nativeConnection.ref(); nativeConnection.unref();", + server: /* js */ ` + import { pgAuthenticationOk, pgReadyForQuery } from ${JSON.stringify(wireFrames)}; + export const url = port => \`postgres://postgres@127.0.0.1:\${port}/db\`; + export function onSocket(socket) { + socket.once("data", () => { + socket.write(Buffer.concat([pgAuthenticationOk(), pgReadyForQuery()])); + }); + socket.on("error", () => {}); + }`, + }, + { + name: "mysql", + // .close() on a failed connection calls clean_queue_and_close() which + // calls self.socket.close() and so reads the freed is_closed flag. + touch: "nativeConnection.close();", + server: /* js */ ` + import { mysqlHandshakeV10, mysqlOkPacket } from ${JSON.stringify(wireFrames)}; + export const url = port => \`mysql://root@127.0.0.1:\${port}/db\`; + export function onSocket(socket) { + let buffered = Buffer.alloc(0), authed = false; + socket.write(mysqlHandshakeV10()); + socket.on("data", chunk => { + buffered = Buffer.concat([buffered, chunk]); + while (buffered.length >= 4) { + const len = buffered[0] | (buffered[1] << 8) | (buffered[2] << 16); + if (buffered.length < 4 + len) break; + const seq = buffered[3]; + buffered = buffered.subarray(4 + len); + if (!authed) { authed = true; socket.write(mysqlOkPacket(seq + 1)); } + // never respond to queries, so they stay in the native request queue + } + }); + socket.on("error", () => {}); + }`, + }, +] as const; + +// The failure is a 1-byte heap-use-after-free; it is only observable under +// ASAN (debug builds enable ASAN). +for (const { name, touch, server } of drivers) { + test.skipIf(!isDebug && !isASAN)( + `${name}: touching the native connection after the socket is freed does not read freed memory`, + async () => { + using dir = tempDir(`sql-conn-socket-uaf-${name}`, { + "server.ts": server, + "fixture.ts": /* js */ ` + import net from "node:net"; + import { SQL } from "bun"; + import { onSocket, url } from "./server.ts"; + + let socketRef; + const server = net.createServer(socket => { + socketRef = socket; + onSocket(socket); + }); + server.listen(0, "127.0.0.1"); + await new Promise(r => server.on("listening", r)); + const { port } = server.address(); + + const sql = new SQL({ url: url(port), max: 1, connectionTimeout: 30 }); + + // Capture the native connection by shadowing the query handle's + // .run(connection, query) — the pool hands it the native handle. + let nativeConnection; + let runCount = 0; + const q = sql\`select 1\`; + q.values(); // force lazy creation of the native query handle + const handleSym = Object.getOwnPropertySymbols(q).find(s => s.description === "handle"); + const handle = q[handleSym]; + const protoRun = Object.getPrototypeOf(handle).run; + Object.defineProperty(handle, "run", { + configurable: true, + writable: true, + value(connection, query) { + nativeConnection = connection; + runCount++; + return protoRun.call(this, connection, query); + }, + }); + const settled = q.catch(err => err?.code ?? String(err)); + + // Wait for the connection to establish and the query to enqueue. + while (runCount === 0) await new Promise(r => setImmediate(r)); + + // Replace the pool's JS onclose so it does not pre-emptively drop + // our reference; we want the native connection to outlive the + // socket by at least one tick. + nativeConnection.onclose = () => {}; + + // Server drops the socket -> native on_close -> status=Failed. The + // us_socket_t goes onto closed_head and is freed at the end of the + // current usockets tick. + socketRef.destroy(); + + // Yield past us_internal_free_closed_sockets. + for (let i = 0; i < 5; i++) await new Promise(r => setImmediate(r)); + + // Without the fix this reads s->flags.is_closed on a freed + // us_socket_t and ASAN aborts with heap-use-after-free. + ${touch} + + await settled; + // A second round of touches after the promise machinery has + // settled covers the re-entrant path the timer originally hit. + ${touch} + + console.log("ok"); + process.exit(0); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.ts"], + env: { + ...bunEnv, + // symbolize=0: the full symbolized report under debug+ASAN can + // take tens of seconds; we only need to see the crash, not the + // stack. + ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=1:symbolize=0", + BUN_ENABLE_CRASH_REPORTING: "0", + }, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + timeout: 20_000, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 }); + }, + 30_000, + ); +} From 59d4ce9f3e73eb9af2196979f9a7fc52d39df835 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 27 Jun 2026 18:08:55 +0000 Subject: [PATCH 2/4] ci: retrigger From b8a8ce005133c80db10a108fa9b1c742e0d5dec0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 27 Jun 2026 18:18:39 +0000 Subject: [PATCH 3/4] test: address review feedback on sql-connection-socket-uaf Use once(server, 'listening') so a listen error rejects instead of hanging, and tighten the header comment. --- test/js/sql/sql-connection-socket-uaf.test.ts | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/test/js/sql/sql-connection-socket-uaf.test.ts b/test/js/sql/sql-connection-socket-uaf.test.ts index 10129f3c84a0..c6a80b7872e3 100644 --- a/test/js/sql/sql-connection-socket-uaf.test.ts +++ b/test/js/sql/sql-connection-socket-uaf.test.ts @@ -4,18 +4,11 @@ // All wire-protocol bytes come from test/js/sql/wire-frames.ts; do not inline // Buffer.alloc frame construction here. // -// PostgresSQLConnection/MySQLConnection kept the raw us_socket_t* in -// self.socket after usockets dispatched on_close/on_connect_error. usockets -// frees closed sockets at the end of the same tick, so any later read of -// self.socket.is_closed() (connection.ref()/.unref()/.close(), or the -// connectionTimeout timer) was a heap-use-after-free. Valkey already -// detaches its stored socket in the same callbacks; Postgres and MySQL now -// do too. -// -// The fixture captures the native connection by shadowing the query -// handle's .run(connection, query), lets the server drop the socket, yields -// past us_internal_free_closed_sockets, then touches the native connection -// through a method that reads the stored socket pointer. +// The SQL drivers kept the raw us_socket_t* in self.socket after +// on_close/on_connect_error; usockets frees it at end-of-tick, so later +// reads (timer callbacks, ref()/unref()/close()) were heap-use-after-free. +// Capture the native handle via the query handle's .run(connection, …), +// drop the server socket, yield past the free, then touch the handle. import { expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN, isDebug, tempDir } from "harness"; @@ -76,6 +69,7 @@ for (const { name, touch, server } of drivers) { "server.ts": server, "fixture.ts": /* js */ ` import net from "node:net"; + import { once } from "node:events"; import { SQL } from "bun"; import { onSocket, url } from "./server.ts"; @@ -85,7 +79,7 @@ for (const { name, touch, server } of drivers) { onSocket(socket); }); server.listen(0, "127.0.0.1"); - await new Promise(r => server.on("listening", r)); + await once(server, "listening"); const { port } = server.address(); const sql = new SQL({ url: url(port), max: 1, connectionTimeout: 30 }); From 384bf5aa92d906095825d0a63e3790be4802abf1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 27 Jun 2026 18:38:59 +0000 Subject: [PATCH 4/4] test: drop redundant per-test timeout and relax stderr assertion The Bun.spawn timeout already bounds the subprocess, and the default test timeout covers the pass case. Use expect.any(String) for stderr (like the sibling sql-onconnect-onclose-throw and sql-mysql-columns-realloc-oom tests) so benign debug/ASAN noise does not spuriously fail the test; the real signal is stdout and exitCode. --- test/js/sql/sql-connection-socket-uaf.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/js/sql/sql-connection-socket-uaf.test.ts b/test/js/sql/sql-connection-socket-uaf.test.ts index c6a80b7872e3..5a6800a9846c 100644 --- a/test/js/sql/sql-connection-socket-uaf.test.ts +++ b/test/js/sql/sql-connection-socket-uaf.test.ts @@ -152,8 +152,13 @@ for (const { name, touch, server } of drivers) { const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 }); + // stderr is kept in the diff via expect.any(String); the failure + // signal is the missing "ok\n" and non-zero exitCode (ASAN aborts). + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "ok\n", + stderr: expect.any(String), + exitCode: 0, + }); }, - 30_000, ); }