Repository navigation
tls: keep a stream TLS engine's context alive while its entry points run - #46335
Conversation
DuplexUpgradeContext is now refcounted. The open connection holds one ref, and each UpgradedDuplex entry point holds one while it runs. The task that frees the context is queued when the count reaches zero. Before, the close of the socket queued that task at once. A callback that turned the event loop synchronously ran the task while the TLS engine, BoringSSL and UpgradedDuplex were still on the stack.
…lback turns the event loop
|
Status: reproduced and fixed.
|
|
Updated 11:14 AM PT - Oct 21st, 2026
✅ @robobun, your commit ba75f868b546040453c7bffb8abbc8c46982ba9f passed in 🧪 To try this PR locally: bunx bun-pr 46335That installs a local version of the PR into your bun-46335 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe duplex TLS context now uses reference counting and queued cleanup. Runtime entry points retain the context while active. A testing API and lifecycle tests report live context counts across TLS-over-duplex callbacks and connection-ending cases. ChangesTLS duplex context lifetime
Possibly related issues
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: keeping the stream TLS engine context alive while entry points execute. |
| Description check | |
The description provides detailed problem, fix, verification results, trade-offs, and test coverage. It does not use the template headings "What does this PR do?" and "How did you verify your code wor… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides detailed problem, fix, verification results, trade-offs, and test coverage. It does not use the template headings "What does this PR do?" and "How did you verify your code works?", but it contains the required information under equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Problem
AddressSanitizer: heap-use-after-freeinus_ssl_wrapper_keylogunderSSL_do_handshake, or inSSLWrapper::flush_pending_events. Release builds end withpanic: RefCell already borrowedor stay silent.DuplexUpgradeContext::on_close(src/runtime/socket/socket_body.rs) queued the task that frees the context at once. The nested turn ran it under the TLS engine.Fix
DuplexUpgradeContextis refcounted. The open connection holds one ref, and eachUpgradedDuplexentry point holds one while it runs. The free task is queued at zero.test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts. Ten cases abort on the base build and pass with the fix. Also twelve TLS, http2 and module-graph suites (Notes).Background
DuplexUpgradeContextownsUpgradedDuplex, which owns the TLS engine (SSLWrapper) behindtls.connect({socket}).expect(promise).resolvesandBun.buildwith an async pluginsetup()run queued tasks while they wait.WindowsNamedPipe(net: keep a Windows named-pipe socket alive while its read callbacks run #39983) holds a ref at its entry points.Downsides
Notes
What turns the loop, and what closes the socket
EventLoop::wait_for_promise(src/jsc/event_loop.rs). It callstick(), which runs the task queue. The fixture uses two callers: abun:testpromise matcher in a plain script, andBun.buildwith an async pluginsetup()(src/runtime/api/JSBundler.rs).destroy()in a later turn (the callback is the transport'sfinal()), a transport that closes during the turn, and aBun.ModuleGraphthat is disposed.SSLuntil the pass ends). The owner was still freed by the queued task.The ten cases on the base build (debug, ASAN)
Each one ends in
heap-use-after-free. The free isDuplexUpgradeContext::deinitfromrun_event, or fromrelease_unrunfor the disposed graph.us_ssl_wrapper_keylog<us_ssl_keylog_cb<SSL_do_handshakeSSLWrapper::flush_pending_events<traffic_passUpgradedDuplex::is_shutdown<UpgradedDuplex::on_handshakeSSLWrapper::dispatch_read<handle_readingBun.build()SSLWrapper::dispatch_read<handle_readingSSLWrapper::dispatch_read<handle_readingwrite()SSLWrapper::handle_writing<write_datafinal(),destroy()in a later turnUpgradedDuplex::finish_close<on_closeSSLWrapper::dispatch_read<handle_readingBun.ModuleGraphis disposedSSLWrapper::dispatch_read<handle_readingRelease build of the base, original repro (ALPNCallback destroys the socket, then
expect().resolves):panic: RefCell already borrowed, 5 of 5 runs. The 'keylog' variant printed OK 5 of 5 (silent). Sentry was not queried.How the tests decide
duplexTLSInternals.liveCount(), so the tests do not need ASAN to fail.{ before: 3, afterTurn: 2, afterClose: 0 }. With no turn it is 3, and with the fault it is 1.tick()runs. One case turns withBun.build()instead, so the test does not depend on the matcher alone.teardown()inrelease_unrun: a string from the transport reaches the disposed graph asERR_STREAM_WRAP.abort_handle.leave():DuplexUpgradeContext entered after its last ref was released.release_unrun: the graph case reaches it with a count of 1, so an unconditional free trips the assertion indeinit.Direction
bun:testmatchers off the nested wait.REVIEW.md("bracket entry points that can reach synchronous teardown with ref()/defer deref()") and of net: keep a Windows named-pipe socket alive while its read callbacks run #39983.Bun.buildstill waits for an asyncsetup()(Bun.build: stop blocking the event loop on an async plugin setup() #33271 was closed without a merge).Sequencing
UpgradedDuplexand need the guard after it:UpgradedDuplex::resolve_server_name(an SNICallback that answers in a later turn) needs thekeep_alive()guard as its first statement, andDuplexUpgradeContext::on_server_nameneeds thedebug_assert_kept_alive()call. Without them the debug build aborts on the assertion.on_write_donethunk needs the guard.unsafewith typed ownership #40139 rewrites the same lines and keeps the unconditional free in theClosearm. A rebase must keep the rule: theClosehop is queued only at zero.Not in this change
WindowsNamedPipe: net: keep a Windows named-pipe socket alive while its read callbacks run #39983 guardson_read,on_read_error,on_error,on_handshakeandget_accepted_by. Seven entries that dispatch have no guard:on_writable,flush,on_timeout,encode_and_write,raw_write,close,shutdown. No fault is known there: its transport is libuv and runs no script in write, end, pause or resume. Read from the code, not run on Windows. Do not copykeep_aliveto those entries as is: there, a ref taken at zero queues the free twice.ProxyTunnel(src/http/ProxyTunnel.rs) runs on the HTTP thread, where no script runs.StartTLSarm now releases the connection's ref. It has no test.node:tlsandnode:http2pass a prebuiltSSL_CTX, so the start fails only ifSSL_newfails.Measurements (linux x64, base 71bc818)
perfis not installed here. The counts areperf_event_open(PERF_COUNT_HW_INSTRUCTIONS, user only) around the release binaries. One TLS echo over an in-process Duplex pair, 100,000 round trips of 1 KiB,BUN_JSC_useJIT=0, 10 interleaved runs each.UpgradedDuplex__flush: 4 instructions at entry and 5 at exit. A round trip takes 4 guards (gdb count on the debug build).sizeof(DuplexUpgradeContext): 568 B -> 568 B (gdb on the debug builds). The two fields fit in padding.run_event, 50 cycles): normal close 2 -> 2,destroy()before the engine starts 2 -> 2, transport error before the engine starts 1 -> 2.llvm-size):.text81,904,741 -> 81,909,861 (+5,120),.rodata+4,096, file 107,497,184 -> 107,505,376 (+8,192). Host functions: +1, testing only.Suites (debug build,
--timeout 90000)Same results as the base build:
node-tls-duplex-end-verify(115),node-tls-duplex-write-throw-error-value(1),node-tls-wrapped-socket-close(113),node-tls-server(218),node-tls-upgrade(21),node-http2-upgrade(23),renegotiation(58),node-tls-context(83),tls-fatal-error-closes(38),tls-reject-before-client-cert(126),module-graph-io(28),module-graph-isolation(413).node-tls-connecthas the same 4 failures on both builds: those tests contact bun.sh, which this machine cannot reach.cargo clippy -p bun_runtimeandtest/internal/source-lints/are clean.