sql(postgres,mysql): guard ref_and_close against re-entrant TLS close dispatch

Closed
RO
2mo ago
+172-3

Crash

Sentry BUN-3KKD / BUN-3GMS (29 events, macOS, bun 1.3.14): segfault at address 0x0 in us_internal_ssl_close with two nested refAndClose frames on the stack:

runtime.timer.Timer.All.drainTimers
event_loop.EventLoopTimer.fire
sql_jsc.postgres.PostgresSQLConnection.failWithJSValue  (PostgresSQLConnection.zig:327)
sql_jsc.postgres.PostgresSQLConnection.refAndClose      (PostgresSQLConnection.zig:962)
uws_sys.socket.AnySocket.close                          (socket.zig:570)
sql_jsc.postgres.PostgresSQLConnection.failWithJSValue  (PostgresSQLConnection.zig:327)
sql_jsc.postgres.PostgresSQLConnection.refAndClose      (PostgresSQLConnection.zig:962)
uws_sys.socket.AnySocket.close                          (socket.zig:570)
packages/bun-usockets/src/crypto/openssl.c:857 us_internal_ssl_close

Cause

ref_and_close calls socket.close() on a TLS socket. us_internal_ssl_close synchronously dispatches on_handshake (when the handshake never completed) and then on_close. Both re-enter fail_with_js_value. That function's re-entry guard is if status == Failed return, but when the outer entry was disconnect() the status is Disconnected, so the guard does not trip and a nested ref_and_close runs on the same us_socket_t.

The on_handshake dispatch happens before us_internal_socket_close_raw sets is_closed = 1, so the nested is_closed() check is not sufficient to prevent a second close().

Traced with debug logging on Linux (the nested ref_and_close is reached; on Linux the inner is_closed() happens to be true by the time it runs so the second close is skipped):

refAndClose: is_closed=false status=Disconnected   <- outer, from disconnect()
failed: Connection closed: ConnectionClosed        <- on_close fired synchronously
refAndClose: is_closed=true status=Failed          <- inner, from fail_with_js_value

self.socket is also never detached after close, so once us_internal_free_closed_sockets frees the us_socket_t at the end of the loop iteration the stored handle dangles.

Fix

  • ConnectionFlags::CLOSE_INITIATED: set once before the first socket.close(), checked on every ref_and_close entry. Monotonic; never cleared. Combined with the existing is_closed() check this makes the close path idempotent regardless of which callback re-enters.
  • SocketHandler::on_close detaches self.socket so the stored handle never outlives the us_socket_t. The hasPendingActivity invariant for deferred TLS closes is preserved: self.socket stays Connected(ptr) until on_close actually fires, so a graceful-close connection still reports pending activity until the peer's close_notify arrives.
  • MySQLConnection::close had no is_closed() guard at all and the scopeguard::defer! in fail_with_js_value runs it on every exit path including the early return; it now detaches the stored handle first, closes through a local copy, and skips when CLOSE_INITIATED is already set.

Tests

test/js/sql/sql-postgres-tls-close-reentry.test.ts exercises both re-entry shapes (disconnect on a Connected TLS connection; connection timeout during a stalled TLS handshake) against a Docker-free mock Postgres-over-TLS server, 30/10 iterations under ASAN, and verifies onclose fires exactly once and the wrappers are collectable afterwards.

Gate note: the double-close only reaches us_internal_ssl_close a second time when the on_handshake dispatch runs with status == Disconnected and ssl_handshake_state == PENDING simultaneously. On Linux/epoll those never overlap (by the time the connection is Connected the handshake state is COMPLETED, and close_raw flips is_closed before dispatching on_close); all 29 Sentry events are macOS/kqueue. A 30-iteration probe of both paths under ASAN on the unfixed build did not fire. The regression test is coverage for the re-entry path rather than a deterministic fail-before.

Existing coverage (all green with the change):

  • test/js/sql/sql-close-pending-connection.test.ts
  • test/js/sql/postgres-tls-ctx-leak.test.ts
  • test/js/sql/sql-connect-error-reporting.test.ts
  • test/js/sql/sql-onconnect-onclose-throw.test.ts
  • test/js/sql/sql-mysql-tls-plaintext-injection.test.ts
  • test/js/sql/sql-mysql-clean-reentry.test.ts