Free DuplexUpgradeContext after duplex TLS upgrade closes

Closed
RO
6mo ago
+83-5

What

DuplexUpgradeContext is heap-allocated for every tls.connect({socket: <Duplex>}) (and the HTTP/2 upgrade path) but was never freed.

Why

On close, DuplexUpgradeContext.onClose schedules a .Close task via deinitInNextTick(). That task's handler called this.upgrade.close() — which is a no-op because UpgradedDuplex.onClose has already run defer this.deinit() and nulled the SSL wrapper — instead of this.deinit().

deinit() was therefore unreachable dead code. Proof: it called this.destroy(), which does not exist on the type (TrivialNew only provides .new()), and Zig's lazy analysis never type-checked it.

Fix

  • .Close task branch now calls this.deinit().
  • deinit() uses bun.destroy(this) and also frees a lingering ssl_config for safety.
  • onClose nulls this.tls before forwarding to TLSSocket.onClose. The tls.ref() taken in jsUpgradeDuplexToTLS is the "attached socket" ref balanced by TLSSocket.onClose's defer this.deref(); without nulling, the now-reachable deinit() would deref a second time and free the native TLSSocket while its JS wrapper is still live (UAF on finalize).

Verification

Added a debug-only test in test/js/bun/net/socket-retention.test.ts that runs 5 duplex-upgraded TLS connections in a subprocess with BUN_DEBUG_alloc=1 and asserts new(DuplexUpgradeContext) count equals destroy(DuplexUpgradeContext) count.

Before:

{ created: 5, destroyed: 0 }

After:

{ created: 5, destroyed: 5 }

node-tls-connect.test.ts (duplex proxy suite) and node-http2-upgrade.test.mts pass under ASAN with no use-after-free.