sql: detach the stored socket in on_close/on_connect_error - #32861
sql: detach the stored socket in on_close/on_connect_error#32861robobun wants to merge 4 commits into
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds socket detachment in Socket UAF Fix
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:00 PM PT - Jun 27th, 2026
❌ @robobun, your commit 384bf5a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32861That installs a local version of the PR into your bun-32861 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flag: #32573 and this PR overlap on the Postgres #32573 fixes a separate re-entrant TLS close crash (Sentry BUN-3KKD) and, as part of that, detaches in Postgres This PR detaches in The two can merge in either order; whichever lands second will have a trivial rebase on the shared Postgres |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested UAF fix that mirrors the existing Valkey detach pattern.
Extended reasoning...
Overview
This PR adds a single line (plus comment) at the top of four socket callbacks — on_close and on_connect_error in both JSMySQLConnection.rs and PostgresSQLConnection.rs — replacing the stored us_socket_t* with SocketTCP::detached() before any user-visible callback runs. It also adds a deterministic ASAN regression test (sql-connection-socket-uaf.test.ts) that captures the native connection handle, drops the server socket, yields past us_internal_free_closed_sockets, and then touches the handle.
Security risks
None introduced. The change removes a heap-use-after-free: previously the stored raw socket pointer remained set after usockets queued it for free at end-of-tick, so later is_closed() / close() / ref() calls dereferenced freed memory. A detached socket reports is_closed() == true without dereferencing and makes write()/close() no-ops, so downstream code (the fail → clean_queue_and_close path, timer callbacks, update_has_pending_activity) behaves identically minus the UAF.
Level of scrutiny
Low-to-medium. Although this is native socket-lifecycle code, the fix is purely additive, mechanical, and copies the exact pattern already shipping in src/runtime/valkey_jsc/js_valkey.rs (lines ~2022 and ~2052) for the same two callbacks. set_socket is a trivial field assignment, and nothing in the subsequent fail/on_close paths needs the live socket pointer — they only call close() on it, which is already a no-op for an about-to-be-freed closed socket and remains a no-op for a detached one.
Other factors
- No CODEOWNERS cover these paths.
- The bug-hunting system found no issues.
- The PR description lists four existing tests exercising these callbacks that still pass, and the new test is ASAN-gated so it won't flake on release builds.
- The test relies on monkey-patching the query handle's
.runvia a symbol lookup, which is a bit fragile to internal refactors, but that's acceptable for a fault-injection regression test and is clearly commented as a do-not-copy pattern.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/sql/sql-connection-socket-uaf.test.ts`:
- Around line 1-18: Trim the header comment in the sql-connection-socket-uaf
regression test to 3 lines max and remove PR/bug-history details. Keep only a
brief note about the fault-injection purpose and the socket use-after-free
behavior, referring to the existing test fixture and the native
connection/socket handling in sql-connection-socket-uaf.test.ts.
- Around line 78-88: The startup wait in the SQL socket test only listens for
"listening", so a failed server.listen() can hang instead of rejecting. Update
the server startup await in the net.Server fixture to use once(server,
"listening") so it also attaches the "error" listener and rejects on listen
failure. Keep the change localized to the test setup around server.listen(),
socketRef, and onSocket.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 601cbbea-bc94-4845-a377-3667af1c7912
📒 Files selected for processing (3)
src/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/js/sql/sql-connection-socket-uaf.test.ts
Use once(server, 'listening') so a listen error rejects instead of hanging, and tighten the header comment.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/sql/sql-connection-socket-uaf.test.ts:161-163— Two test-convention nits on adjacent lines: (1) line 163 passes30_000as an explicit per-test timeout —test/CLAUDE.mdsays "Do not set a timeout on tests", and theBun.spawnalready hastimeout: 20_000so it's redundant; (2) line 161 assertsstderr: ""exactly in a test that only runs under debug/ASAN — rootCLAUDE.mdsays not to require stderr to be exactly empty since those builds can emit benign noise. Consider dropping the30_000and usingstderr: expect.not.stringContaining("AddressSanitizer")(orexpect.any(String)) like the sibling tests in this directory.Extended reasoning...
What
Two repo-guideline nits in the new test file, both on the final assertion/timeout lines:
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 }); // line 161 }, 30_000, // line 163
(1) Explicit per-test timeout
test/CLAUDE.md:120states:CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
The test passes
30_000as the third argument totest.skipIf(...)(...). It's also redundant in practice: the subprocess is already bounded byBun.spawn({ ..., timeout: 20_000 })at line 156, so the test body cannot run longer than ~20s of subprocess time plus a fewawaits. Dropping the30_000brings it in line with the guideline without changing behaviour. (A handful of existing files intest/js/sql/do set explicit timeouts, so this isn't strictly enforced — but no reason to add a new one when the spawn timeout already covers it.)(2) Exact-empty stderr under debug/ASAN
Root
CLAUDE.md:195(Landing PRs → Subprocess tests) says:Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined
{ stdout, stderr, exitCode }object.This test is gated by
skipIf(!isDebug && !isASAN)— i.e. it only runs in the configurations the guideline is warning about. The combined-object shape is right, butstderr: ""means any benign diagnostic on stderr (mimalloc warning, debug-build chatter, ASAN informational line) would fail the test even though the UAF fix is working. The actual failure signal here is an ASANheap-use-after-freereport plus a non-zero exit code — neither of which requires stderr to be byte-for-byte empty.Sibling tests in
test/js/sql/use the more robust pattern:sql-onconnect-onclose-throw.test.ts:106→stderr: expect.any(String)sql-mysql-columns-realloc-oom.test.ts:112→stderr: expect.any(String)- several others filter stderr for
"AddressSanitizer"before asserting.
Step-by-step example of how the stderr assertion could spuriously fail:
- CI runs the debug+ASAN job;
isDebugis true so the test runs. - The fixture works correctly: the socket is detached, no UAF, the child prints
ok\nand callsprocess.exit(0). - The debug allocator or ASAN runtime emits a one-line informational warning to stderr (e.g. an env-var parse note or a mimalloc message) — not an error, just noise.
exitCodeis0,stdoutis"ok\n", butstderris"<warning>\n"instead of"".toEqual({ ..., stderr: "" })fails → red CI for a passing fix.
Addressing the counter-argument: one verifier noted that
sql-mysql-clean-reentry.test.ts:137(andnode-tls-duplex-close-throw-uaf.test.ts) already assertstderr: ""under the same gating with the samebunEnv+ASAN_OPTIONS+process.exit(0)setup, and they're green — so the practical flake risk today is low. That's fair, and is why this is filed as a nit rather than a blocking issue:bunEnvsetsBUN_DEBUG_QUIET_LOGS=1andprocess.exit(0)skips atexit leak summaries, which together suppress most known noise sources. But the repo guideline is explicit, the more-robust pattern is already the dominant one in this directory, and matching it costs nothing.Suggested change
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: expect.not.stringContaining("AddressSanitizer"), exitCode: 0, }); }, // drop the 30_000 );
This keeps stderr in the failure diff (so an actual ASAN report is visible when the test fails), asserts on the real failure signal, and removes the redundant timeout.
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.
|
Addressed in 384bf5a: dropped the per-test |
|
CI status (build 65627, final): every red lane is infra or an unrelated pre-existing flake; the diff is ready.
Everything that runs the new test (all x64-asan shards) is green, and none of the red tests touch |
There was a problem hiding this comment.
LGTM — small, pattern-matching UAF fix mirroring the Valkey client, with a deterministic ASAN regression test.
Extended reasoning...
Overview
This PR adds a single statement to four callback sites — SocketHandler::on_close and SocketHandler::on_connect_error in both JSMySQLConnection.rs and PostgresSQLConnection.rs — that replaces the stored us_socket_t* with a detached sentinel before any downstream logic runs. usockets frees closed sockets at end-of-tick, so the stale pointer was a heap-use-after-free if touched afterward (timer callbacks, ref()/unref()/close(), update_has_pending_activity). A new fault-injection test (test/js/sql/sql-connection-socket-uaf.test.ts) deterministically reproduces the ASAN failure without the fix.
Security risks
None. This strictly narrows the lifetime of a raw pointer; a detached SocketTCP reports is_closed() == true and no-ops on write()/close(), so the change cannot introduce new behavior beyond making the already-closed socket observe as closed sooner. No auth, crypto, or input handling is touched.
Level of scrutiny
The native change is tiny and mechanical: four one-line additions, each identical in shape to what the Valkey client already does at src/runtime/valkey_jsc/js_valkey.rs:2022 and :2052. The fix is defensive (overwriting a stored pointer with a sentinel) and sits at the very start of each handler, before any control flow that could re-enter — there is no ordering subtlety with the existing DerefOnDrop guard in MySQL's on_close since set_socket is a plain store. The PR description includes the ASAN signature and confirms the existing close/connect-error tests still pass.
Other factors
All CodeRabbit feedback (comment trimming, once(server, "listening"), dropping the redundant per-test timeout, relaxing the stderr assertion to expect.any(String)) has been addressed and the threads are resolved. The bug-hunting system found nothing. The musl build failures reported on an earlier commit are in the build-bun step itself and unrelated to four added Rust statements that compile on every other shard. The known overlap with #32573 on the Postgres on_close hunk is a trivial rebase, not a correctness concern. No CODEOWNERS apply.
…ed (#33016) A backend message that fails the connection can share a TCP read with messages that follow it. `PostgresRequest::on_data`'s message loop had no bail-out once `fail()` had run, so the trailing messages in that read kept being dispatched against the already-failed connection. ### Repro A mock backend that answers the StartupMessage with one write carrying two messages: ``` R int32(8) int32(99) Authentication, unrecognized type Z int32(5) 'I' ReadyForQuery ``` ```ts const sql = new SQL({ url: `postgres://u@127.0.0.1:${port}/db`, max: 1, idleTimeout: 1, connectionTimeout: 5 }); await sql`select 1`.catch(() => {}); await Bun.sleep(1600); ``` ### Cause The unrecognized `Authentication` type calls `fail()`, which sets the status to `Failed`, closes the socket, and rejects the pending requests, but the message loop keeps going and dispatches the `ReadyForQuery` from the same read. That calls `set_status(Status::Connected)`, which has no guard against leaving `Failed`, so the dead connection is flipped back to `Connected` and the `on_data` epilogue re-arms its idle timer. uSockets frees a closed `us_socket_t` at the end of the event-loop iteration, so when the timer later fires, `ref_and_close` reads the freed socket: ``` ERROR: AddressSanitizer: heap-use-after-free READ of size 1 at 0x71f2125605d2 thread T0 #0 us_socket_is_closed packages/bun-usockets/src/socket.c:143:21 #4 PostgresSQLConnection::ref_and_close src/sql_jsc/postgres/PostgresSQLConnection.rs:1528:31 #5 PostgresSQLConnection::fail_with_js_value src/sql_jsc/postgres/PostgresSQLConnection.rs:726:14 #6 PostgresSQLConnection::fail_fmt src/sql_jsc/postgres/PostgresSQLConnection.rs:749:14 #7 PostgresSQLConnection::on_connection_timeout src/sql_jsc/postgres/PostgresSQLConnection.rs:557:14 #8 __bun_fire_timer src/runtime/dispatch.rs:1020:35 0x71f2125605d2 is located 18 bytes inside of 104-byte region freed by thread T0 here: #2 us_internal_free_closed_sockets packages/bun-usockets/src/loop.c:305:9 ``` ### Fix - `PostgresRequest::on_data`: the message loop returns once the connection's status is `Failed`. `fail()` is terminal; nothing after it in the same read should be handled (a `DataRow`, `CommandComplete`, or `ErrorResponse` in that position would be just as wrong as the `ReadyForQuery`). - `PostgresSQLConnection::set_status`: refuses to transition out of `Failed`. The transition function owns that invariant; every other consumer of `Status` (the timer interval, `update_has_pending_activity`, the idempotency check in `fail_with_js_value`) already assumes `Failed` is terminal. ### Verification `test/js/sql/postgres-failed-connection-resurrection.test.ts` runs a fixture against the mock backend above and lets it outlive the idle-timer window. Without the fix the fixture dies with the ASan report above; with it the fixture exits 0. Gated to ASan builds because the bug is a read of freed memory, which release lanes do not detect. The postgres fault-injection and integration suites still pass locally (90 tests across `test/js/sql/postgres-*.test.ts`, `sql*.test.ts`, `tls-sql.test.ts`). ### Related - #32861 detaches the stored socket handle in `on_close` / `on_connect_error` so nothing can dereference the freed `us_socket_t` regardless of how the stale read is reached. It removes the last step of this chain from the other end; this PR stops the failed connection from being resurrected at all. - #30950 guards the JS pool's `handleConnected` against the reverse ordering within one read (a legitimately queued `onconnect` microtask arriving after a synchronous `onclose`).
|
Independent confirmation of the MySQL half of this from a new fuzz report, via a different trigger: instead of the server dropping the socket, a scripted server completes the handshake and auth, then answers the first post-auth command with a byte that is neither Reproduced 3/3 under ASan on today's I verified by reading the code that the For completeness, #32573 independently fixes the same MySQL path from the other end, by detaching inside If it is useful as a second test variant (client-initiated teardown, distinct from the socket-drop one already in malformed-post-auth-frame variantimport net from "node:net";
import { SQL } from "bun";
import { mysqlHandshakeV10, mysqlOkPacket, mysqlRawPacket } from "./wire-frames.ts";
// Handshake + auth OK, then answer the first post-auth command with a garbage
// byte that is neither OK (0x00) nor ERR (0xff). The client reports
// UnexpectedPacket and tears the connection down itself.
const server = net.createServer(socket => {
let buffered = Buffer.alloc(0);
let 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));
} else {
socket.write(mysqlRawPacket(seq + 1, Buffer.from([0xaa, 0xaa, 0xaa, 0xaa])));
}
}
});
socket.on("error", () => {});
});
server.listen(0, "127.0.0.1");
await new Promise(r => server.on("listening", r));
const { port } = server.address();
const sql = new SQL({ url: `mysql://root@127.0.0.1:${port}/db`, max: 1 });
const q = sql`SELECT 1`;
q.values();
const handleSym = Object.getOwnPropertySymbols(q).find(s => s.description === "handle");
const handle = q[handleSym];
const protoRun = Object.getPrototypeOf(handle).run;
let nativeConnection;
Object.defineProperty(handle, "run", {
configurable: true,
writable: true,
value(connection, query) {
nativeConnection = connection;
return protoRun.call(this, connection, query);
},
});
const code = await q.then(() => "resolved", e => e?.code ?? String(e));
// Yield real event-loop iterations so the post-close sweep has freed the socket.
await new Promise(r => setImmediate(r));
await new Promise(r => setImmediate(r));
// doClose takes the Failed arm and must not touch the freed us_socket_t.
nativeConnection.close();
await new Promise(r => setImmediate(r));
console.log("ok", JSON.stringify({ code, connected: nativeConnection.connected }));
// expected: ok {"code":"ERR_MYSQL_UNEXPECTED_PACKET","connected":false}
server.close();
process.exit(0); |
1 participant
What
PostgresSQLConnectionandJSMySQLConnectionkept the rawus_socket_t*inself.socketafter usockets dispatchedon_close/on_connect_error. usockets frees closed sockets atus_internal_loop_postlater in the same tick, so any subsequent read throughself.socket(theconnectionTimeoutEventLoopTimer,ref()/unref()/close()on the native handle,update_has_pending_activity) dereferenced freed memory.ASan signature from the reproduction:
Fix
Detach the stored handle at the start of
SocketHandler::on_closeandSocketHandler::on_connect_errorfor both drivers, matching what the Valkey client already does insrc/runtime/valkey_jsc/js_valkey.rs. A detached socket reportsis_closed() == truewithout dereferencing anything, andwrite()/close()on it are no-ops.Test
test/js/sql/sql-connection-socket-uaf.test.tscaptures the native connection handle by shadowing the query handle's.run(connection, query), lets the mock server drop the socket, yields pastus_internal_free_closed_sockets, then calls.ref()(Postgres) /.close()(MySQL) on the native handle. Under ASan this deterministically reads the freedis_closedflag without the fix and exits cleanly with it.Existing coverage that exercises the modified callbacks still passes:
test/js/sql/sql-connect-error-reporting.test.tstest/js/sql/sql-close-pending-connection.test.tstest/js/sql/sql-mysql-clean-reentry.test.tstest/js/sql/postgres-tls-ctx-leak.test.tsRelated: #32573 fixes a separate re-entrant TLS close crash and also detaches in Postgres
on_close; this PR additionally coverson_connect_errorfor both drivers and adds a deterministic ASan regression.