http: release h2/h3 sessions and proxy tunnels through their holders' pointers, not &mut receivers - #37870
Conversation
… pointers ClientSession::on_close and maybe_release (h2), ProxyTunnel::detach_and_deref and h3 ClientSession::detach released an intrusive ref by passing their own &mut self receiver to deref(). For the h2 and tunnel sites that release is normally the last one, so the allocation was freed while a reference argument to it was still live, which both aliasing models reject (protected tag). The h2 ref_scope() guards and the tunnel's receive/on_writable guards had the same shape one step later: built from the receiver, dropped inside the method, and the last release whenever the body released the other holders' refs. h2: the socket events, adopt, the leader attach, enqueue and the per-request wakeups now take a SessionPtr (ThisPtr) and run their &mut bodies through ClientSession::enter, which holds a guard from that pointer, runs the body through a call-scoped reborrow, and afterwards releases the socket-ext ref the body gave up (socket_ref_owed) and its own. The registry releases its ref through the entry it stored. ActiveSocketExt no longer hands out a &mut ClientSession, and connect() adopts after its registry scan has finished. proxy: receive/on_writable take the client's handle pointer; detach_and_deref is gone (start() builds the wrapper before allocating the tunnel, the pool fallback releases through its RefPtr like close_proxy_tunnel does); adopt() moves the pool's RefPtr into the client instead of re-deriving one from its receiver. h3: detach's release is provably never the last one (the connection ref is released only after pending is drained); it now goes through the stream's own session backref and says why &mut self is fine there. A source lint bans the bare-receiver spellings (Type::deref(self), ScopedRef/RefGuard::new(self)); the two subprocess PipeReader sites being converted separately are allowlisted at their exact count.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Disabled knowledge base sources:
WalkthroughHTTP/2 session callbacks now use owning ChangesHTTP session ownership
Possibly related PRs
Suggested reviewers: Comment |
|
Status Reproduced as a source-tree fact rather than a crash: at 9a543cc, Fix: #37870 (this PR). The h2/h3/proxy suites listed in the description pass on the debug ASAN build. Relationship to #31788 is described in the comment below; the overlap is one line. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #31788, but they touch one line in common, so noting the relationship. #31788 fixes a custom-TLS-context eviction cluster (three defects: the re-entrant teardown reaching The ASAN trace in #31788 is also the one observed instance of the shape this PR is about: the session freed by |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it restructures intrusive-refcount release ordering across the h2/h3/proxy-tunnel paths — memory-safety-critical native code with subtle aliasing-model reasoning — a human look is still warranted.
Checked: the new ClientSession::enter pattern's guard/release ordering (the socket_ref_owed deferred release runs while _keep_alive still holds a ref, so it's never the last); connect()'s registry scan now completes before adopt can swap-remove from active_h2_sessions; ProxyTunnel::start's reordering (wrapper built before tunnel alloc) so the error path has nothing to release; and the h3 detach release going through the stream's backref with the never-last-ref invariant documented.
Extended reasoning...
Overview
This PR eliminates a class of aliasing-model violations where intrusive-refcount releases (Type::deref(self)) and keep-alive guards (ScopedRef::new(self)) were performed through a method's own &mut self receiver — freeing the allocation while a live reference argument still points at it, which both Stacked Borrows and Tree Borrows reject. It touches 7 source files in src/http/ (h2 ClientSession, h3 ClientSession, ProxyTunnel, HTTPContext, HTTPThread, H2Client, lib.rs) plus a new source-lint test.
The h2 fix introduces a SessionPtr (ThisPtr<ClientSession>) type and an enter() wrapper: every public entry point that can leave the session released now takes the holder's pointer, runs the body through a scoped &mut reborrow under a keep-alive guard, and performs the socket-ext ref release (via a new socket_ref_owed: Cell<bool>) after the borrow ends. Ten entry points are converted; ActiveSocketExt::session_mut becomes session() -> Option<SessionPtr>. The proxy-tunnel fix converts on_writable/receive to take NonNull<Self>, deletes detach_and_deref, reorders start() to build the SSL wrapper first, and makes adopt take the pool's RefPtr by value. The h3 change is documentation plus releasing through the stream's backref.
Security risks
None identified. This is a memory-safety refactor of internal refcount plumbing; no user-facing API, input parsing, or auth surface changes. The changes strictly tighten aliasing invariants.
Level of scrutiny
High. This is native memory-safety code in the HTTP client hot path. The refactor changes when and through which pointer heap allocations are freed, restructures the ownership contract between socket ext slots / registry / pool / guards, and adds a deferred-release mechanism (socket_ref_owed). Getting the ordering wrong in any converted entry point could turn a theoretical Miri violation into a real UAF. The PR description is exceptionally thorough and the verification (ASAN + full h2/h3/proxy test suites) is appropriate, but the change is large enough (~400 lines across tightly-coupled modules) and the reasoning subtle enough that a maintainer familiar with the bun_ptr refcount traits and the HTTP thread's ownership model should confirm the enter pattern and the connect() iteration reordering.
Other factors
- The new source-lint test is well-constructed (positive/negative pattern checks, ratcheted allowlist for the two
SubprocessPipeReader.rssites being converted separately). - No prior human reviews on the PR; only a rate-limited CodeRabbit placeholder.
- The PR explicitly notes what it does not fix (
NonNull::from(&mut *self)inmaybe_release's pool path, re-entrantHTTPContextborrows) — those are worth a maintainer's eye on whether they belong here or in follow-ups.
|
Updated 1:56 PM PT - Aug 12th, 2026
🔄 @robobun, the build for |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// | ||
| /// An h2 session is handed out as a [`h2::SessionPtr`] rather than a `&mut`: | ||
| /// its entry points release the slot's ref themselves (the socket closing, or | ||
| /// the last request finishing on an unpoolable connection), so a `&mut` | ||
| /// argument to them could be freed while still live. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `client_mut`/`pooled_mut` share one SAFETY argument instead of two | ||
| /// open-coded ones. INVARIANT: see [`ActiveSocketExt`] trait doc. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // INVARIANT: see the trait doc — while tagged as the session the slot | ||
| // holds the socket-ext ref, which is what `this_ptr` requires. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// entry to `Option<&'a mut h2::ClientSession>`, for the field writes and | ||
| /// idle-frame handling that cannot release the session. Anything that can | ||
| /// (`adopt`, the socket events) goes through a [`h2::SessionPtr`] instead. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// on the session (taken in `release_socket`, released in | ||
| /// `add_memory_back_to_pool` or handed to the socket ext by `existing_socket` / | ||
| /// `connect`); the session is a distinct heap allocation that outlives the | ||
| /// holder. HTTP-thread-only, so no concurrent `&mut`. Centralises the SAFETY | ||
| /// argument shared by `PooledSocket::h2_session_mut` and | ||
| /// `ExistingSocket::h2_session_mut`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Tail of [`Self::unregister_h2_raw`]: swap-remove the entry at `idx` | ||
| /// from `list`, fix up the swapped-in entry's index, and release the ref | ||
| /// taken in [`Self::register_h2`] through the pointer the registry held. | ||
| /// `session` only identifies the entry being removed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Take `session` out of `active_h2_sessions`, releasing the registry's | ||
| /// ref. Takes the context as a raw pointer because it is reached on | ||
| /// re-entrant call paths (`connect` → `adopt` → `maybe_release` / | ||
| /// `fail_all` → `fail_streams`) where an ancestor stack frame already | ||
| /// holds `&mut HTTPContext<SSL>`. Upgrading the session's `ctx` backref to | ||
| /// a second `&mut Self` there would alias; this entry point instead | ||
| /// projects `active_h2_sessions` through a raw place expression so no | ||
| /// intermediate `&mut Self` is formed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// be live for the duration of the call; it is only used to find and | ||
| /// identify the registry entry, which is what gets released. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `t` is the strong ref the caller transferred; releasing it | ||
| // through the handle (usually the last ref) mirrors | ||
| // `HTTPClient::close_proxy_tunnel`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Listed sessions are kept alive by the registry's ref. The | ||
| // scan only reads; `adopt` runs after the iteration is over | ||
| // because it may end by unregistering the session it was | ||
| // given (swap-removing it from this Vec) and releasing it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Same guard as the pool path: a session whose TLS | ||
| // handshake ran with reject_unauthorized=false never | ||
| // validated the peer hostname, so a strict caller | ||
| // must not multiplex onto it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The `&mut` ends with the statement: `register_h2` | ||
| // forms a fresh `&*session`, which under Stacked |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The pool's ref (carried by `found`) becomes the | ||
| // socket ext's; `adopt` goes through that handle and | ||
| // may release it again if the session turns out to be | ||
| // unusable, so nothing touches `session` afterwards. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `adopt` moves the pool's strong ref into | ||
| // `client.proxy_tunnel`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The resume may tear the session down and | ||
| // release the socket's ref; hold one across | ||
| // the second call. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// INVARIANT (module): `this` is the `HTTPClient.proxy_tunnel` handle's | ||
| /// pointer (the callbacks read it from the client; `on_writable` / | ||
| /// `receive` are passed it), so the tunnel is live and the guard's | ||
| /// eventual release, which is the last one whenever the guarded call | ||
| /// released the client's ref, goes through the holder's own pointer | ||
| /// rather than through a `&mut self` that would still be live at that | ||
| /// point. `ScopedRef::new` bumps via raw `CellRefCounted::ref_count_raw` | ||
| /// field projection — touching only `ref_count`, never the whole tunnel — | ||
| /// so it does not alias the caller's `&SSLWrapper` (see ALIASING NOTE). | ||
| /// HTTP-thread-only. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `RefPtr::new` owns the tunnel's initial ref (`ref_count == 1` from | ||
| // `Default`); the client holds it until `close_proxy_tunnel` or the | ||
| // hand-off to the keep-alive pool. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `this` is not used below: start() synchronously fires on_open() / | ||
| // write_encrypted(), which re-derive `&mut HTTPClient` from the raw ctx | ||
| // pointer and touch only tunnel fields disjoint from `wrapper` via | ||
| // `addr_of!` (see ALIASING NOTE), so the `&SSLWrapper` formed here is | ||
| // never aliased by a `&mut`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Outer socket became writable. `this` is the client's `proxy_tunnel` | ||
| /// handle (see [`Self::ref_scope`]): the flush below can complete or fail | ||
| /// the request, which releases that handle, leaving the guard's ref as the | ||
| /// last one. A `&mut self` receiver would then be freed while still live. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Encrypted bytes arrived on the outer socket. Same contract as | ||
| /// [`Self::on_writable`]: delivering the decrypted data can release the | ||
| /// client's handle, so the guard taken from it may hold the last ref. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Forget the outer socket. Holders call this before releasing their | ||
| /// handle (`RefPtr::deref`), so a tunnel that outlives them (a deferred | ||
| /// `on_close` deref is still pending) never retains a dangling socket. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// `tunnel` is the pool's handle; it moves into `client.proxy_tunnel` as | ||
| /// is, so the ref the client later releases is the one the pool held. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `tunnel` holds the strong ref, so the pointee is live for these | ||
| // writes; nothing re-enters the tunnel until the client's next socket | ||
| // event, after this borrow has ended. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Set when a `&mut self` body has torn the session down (`fail_streams`, | ||
| /// or `maybe_release` closing an unpoolable socket) and the socket-ext ref | ||
| /// is therefore due to be released. The body cannot release it itself: at | ||
| /// that point it is normally the last ref, and freeing the session while a | ||
| /// `&mut self` argument to it is live is undefined behaviour even if the | ||
| /// reference is never used again. The [`SessionPtr`] entry point that ran | ||
| /// the body takes the flag and releases the ref once the borrow has ended | ||
| /// (see [`ClientSession::enter`]). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A live session as its holders point at it: the socket ext slot, the | ||
| /// context's registry, the keep-alive pool, or the pointer `create` returned. | ||
| /// | ||
| /// Every `pub(crate)` entry point that can leave the session released (socket | ||
| /// events, the registry / pool hand-offs in `HTTPContext::connect`, the | ||
| /// per-request wakeups from `HTTPThread`) takes one of these instead of | ||
| /// `&mut self` and goes through [`ClientSession::enter`], so the releases | ||
| /// happen through the holder's pointer after the body's `&mut` borrow has | ||
| /// ended. Callers that need the session alive across two entry points hold a | ||
| /// [`bun_ptr::ThisPtr::ref_guard`] of their own across both. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// INVARIANT: `session` is a session the caller holds a ref on (a socket | ||
| /// ext slot's tag, a registry or pool entry, or the pointer `create` | ||
| /// returned), so it is live for as long as the caller uses the handle. | ||
| /// HTTP-thread-only. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Run a `&mut self` body on the session behind `this`, then perform the | ||
| /// releases it asked for. | ||
| /// | ||
| /// The guard keeps the session alive while the body re-enters clients | ||
| /// (delivering bodies, failing requests) and releases the registry ref, so | ||
| /// no release inside the body is ever the last one. `body` receives a | ||
| /// reborrow that ends when it returns; only then is the socket-ext ref it | ||
| /// may have given up (`socket_ref_owed`) released, followed by the guard's | ||
| /// own ref, both through `this`. When the body tore the session down that | ||
| /// second release frees it, with no reference to it live anywhere. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Socket onClose / onTimeout entry point. The socket is already gone, so | ||
| /// every stream fails and the socket ext's ref is released; unless a | ||
| /// caller holds its own guard, the session is freed before this returns. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Multiplex `client` onto an established (registered or pool-resumed) | ||
| /// session; see [`Self::adopt_client`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Open the first stream on a session `create` just returned, for the | ||
| /// client whose connect negotiated h2. Unlike [`Self::adopt`] this does not | ||
| /// wait for the server's SETTINGS: the leader's stream carries the preface. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Called from the HTTP thread's shutdown queue when a fetch on this | ||
| /// session is aborted; see [`Self::abort_request`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// HTTP-thread wake-up from `scheduleRequestWrite`; see | ||
| /// [`Self::stream_request_body`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// HTTP-thread wake-up from `scheduleResponseBodyDrain`; see | ||
| /// [`Self::drain_response_body`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Allocate a session for a socket whose ALPN just selected h2 and list it | ||
| /// in the context's registry. The returned handle carries the socket ext's | ||
| /// ref: the caller tags the socket with it (`tag_as_h2`) and then opens the | ||
| /// leader's stream with [`Self::attach_leader`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Park a request that was coalesced onto this session's connect until the | ||
| /// server's SETTINGS arrive; see [`Self::park`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// JS just enabled `response_body_streaming` on the request, so flush any | ||
| /// body bytes that arrived between metadata delivery and `getReader()`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// New request body bytes (or end-of-body) are available in the request's | ||
| /// ThreadSafeStreamBuffer. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Parse frames into per-stream state, deliver each ready stream to its | ||
| /// client, then pool or close if no streams remain. Structured "parse all | ||
| /// → deliver all" because delivering may free the client. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Tear the session down once its socket is gone (or, via `fail_all`, | ||
| /// about to be closed): leave the registry, fail every parked and attached | ||
| /// request, and hand the socket-ext ref to the enclosing entry point for | ||
| /// release. Runs exactly once per session: the socket is dead afterwards, | ||
| /// so no further socket event reaches it, and every `fail_all` caller | ||
| /// returns straight away. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The socket-ext ref is no longer wanted; `enter` releases it once this | ||
| /// body has returned. See `socket_ref_owed`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
On the deleted test: commit 133215c was authored by Jarred-Sumner, who removed the source-lint file and then merged, so dropping it was a maintainer call rather than an unstated change on my side. The description's Test section describes the branch as it was before that commit. If a lint guarding against receiver releases ( |
…back closes the session (#42647) ### Problem - A server `RST_STREAM` can make the HTTP/2 fetch client read and free a stream twice. A debug ASAN build of main (09bb546) reports `AddressSanitizer: heap-use-after-free` in `ClientSession::handle_data` (`src/http/h2_client/ClientSession.rs:848`, the `s.state != StreamState::Closed` read). The stream was freed by `drop_stream` in `ClientSession::fail_streams` (`ClientSession.rs:943`). A release build has no report, only heap corruption. - It needs a request with a custom TLS context (for example `tls: { serverName }`) whose entry left the 60-entry context cache. That request then holds the last ref. Its terminal callback drops the context, the drop closes the session's socket, and `on_close` runs `fail_streams` inside the deliver loop. ### Fix - While `delivering` is set, `fail_streams` fails each client and leaves the streams in `streams` and `by_http_id`. The deliver loop already removes a stream that has no client, through `remove_stream`, so each stream is freed once. - Outside the loop `fail_streams` behaves as before. - Correct because the loop's tail is safe on a session that is already closed: `maybe_release` returns at the `registry_index` sentinel, and a write to the closed socket writes nothing. - Verified: `test/js/web/fetch/fetch-http2-client.test.ts` (two new ASAN-only tests, 67 pass). With `src/` at main the RST test fails. Also `fetch-http2-leak.test.ts` (7 pass) and `fetch-http2-adversarial.test.ts` (20 pass). ### Background - `ClientSession` is one HTTP/2 connection of the fetch client. `streams` maps a stream id to a heap `Stream`. Each `Stream` points at the `HTTPClient` of one `fetch()`. - `handle_data` parses frames, then delivers each ready stream to its client in a loop, with `delivering` set. A terminal delivery runs the result callback at once, on the HTTP thread. - Custom TLS contexts are refcounted. The cache holds one ref and each request holds one. <details><summary>Notes</summary> This is the part of #31788 that main still needs. #31788 was closed as stale, and all five of its `src/` files conflict with main now. - Its first defect, `abort_by_http_id` with no ref on the session, is fixed on main: since #37870 every entry point goes through `ClientSession::enter`, which holds a `RefPtr`. The abort test from #31788 passes on main. It is included here because main has no test that evicts a TLS context. - Its third part, a `ctx` field on `PendingConnect`, has no reproduction and is not included. ASAN report on main, trimmed: ``` ERROR: AddressSanitizer: heap-use-after-free READ of size 1 thread T2 (HTTP Client) #0 <State as PartialEq>::eq src/http/h2_client/Stream.rs:104 #2 ClientSession::handle_data src/http/h2_client/ClientSession.rs:848 #4 ClientSession::enter src/http/h2_client/ClientSession.rs:242 #6 Handler<true>::on_data src/http/HTTPContext.rs:1386 freed by thread T2 (HTTP Client) here: #12 drop_stream src/http/h2_client/ClientSession.rs:211 #13 ClientSession::fail_streams src/http/h2_client/ClientSession.rs:943 ``` The two tests share one helper. The child holds a stream open on a context made by `serverName`, runs 61 more TLS configs to evict it, writes `evicted` to stderr, and then ends the held request: one test aborts it, the other has the server send `RST_STREAM(CANCEL)`. Both check that a later `fetch()` still works. They run only under ASAN and carry a 30 s timeout, because each child makes 62 TLS handshakes and a regression needs time to print its report. Without the longer timeout the failing run on main shows a timeout at 5 s, not the report. The tests are serial on purpose: each one fills the context cache, which would evict the context that a concurrent test depends on. One older test in the same file changed: `concurrent requests multiplex on one h2 session`. Its server held each stream for 100 ms and the test expected 8 open at once. On a loaded machine it saw 7 (2 of 9 runs of the file). The server now answers when the eighth stream is open, so the test uses no timer. After the change the file passed 4 of 4 runs on the same loaded machine. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/web/fetch/fetch-http2-client.test.ts" bun test v1.4.3 (b993710) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [1309.07ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [1215.67ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [1497.63ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body streams as raw DATA frames [730.67ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [2353.52ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST: request body is delivered as DATA frames [2514.64ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body larger than initial send window [927.77 ... (truncated) release without fix: 2 skipped bun test v1.4.3-canary.1 (f92be71) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [110.34ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [115.04ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response trailers are consumed without breaking the body [99.30ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > connection-specific request headers are stripped before HPACK [111.10ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > cold-start: parallel requests coalesce onto one TLS connect [135.66ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [147.00ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [160.06ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > keep-alive: sequential requests reuse one h2 session [139.51ms] (pass) fetch() over H ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/web/fetch/fetch-http2-client.test.ts" bun test v1.4.3 (b993710) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [1532.08ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [1265.37ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST: request body is delivered as DATA frames [1731.76ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body streams as raw DATA frames [501.02ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [1802.42ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [1743.84ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent ReadableStream uploads route each chunk to its own stream ... (truncated) release with fix: 2 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 710ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[92m Compiling�[0m bun_clap v0.0.0 (/workspace/bun/src/clap) �[1m�[92m Compiling�[0m bu ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/http/h2_client/ClientSession.rs | 12 ++- test/js/web/fetch/fetch-http2-client.test.ts | 118 ++++++++++++++++++++++++--- 2 files changed, 117 insertions(+), 13 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/http/h2_client/ClientSession.rs 2 1 24 test/js/web/fetch/fetch-http2-client.test.ts 1 2 24 ``` </details> <!-- robobun:evidence:end -->
…back closes the session (oven-sh#42647) ### Problem - A server `RST_STREAM` can make the HTTP/2 fetch client read and free a stream twice. A debug ASAN build of main (09bb546) reports `AddressSanitizer: heap-use-after-free` in `ClientSession::handle_data` (`src/http/h2_client/ClientSession.rs:848`, the `s.state != StreamState::Closed` read). The stream was freed by `drop_stream` in `ClientSession::fail_streams` (`ClientSession.rs:943`). A release build has no report, only heap corruption. - It needs a request with a custom TLS context (for example `tls: { serverName }`) whose entry left the 60-entry context cache. That request then holds the last ref. Its terminal callback drops the context, the drop closes the session's socket, and `on_close` runs `fail_streams` inside the deliver loop. ### Fix - While `delivering` is set, `fail_streams` fails each client and leaves the streams in `streams` and `by_http_id`. The deliver loop already removes a stream that has no client, through `remove_stream`, so each stream is freed once. - Outside the loop `fail_streams` behaves as before. - Correct because the loop's tail is safe on a session that is already closed: `maybe_release` returns at the `registry_index` sentinel, and a write to the closed socket writes nothing. - Verified: `test/js/web/fetch/fetch-http2-client.test.ts` (two new ASAN-only tests, 67 pass). With `src/` at main the RST test fails. Also `fetch-http2-leak.test.ts` (7 pass) and `fetch-http2-adversarial.test.ts` (20 pass). ### Background - `ClientSession` is one HTTP/2 connection of the fetch client. `streams` maps a stream id to a heap `Stream`. Each `Stream` points at the `HTTPClient` of one `fetch()`. - `handle_data` parses frames, then delivers each ready stream to its client in a loop, with `delivering` set. A terminal delivery runs the result callback at once, on the HTTP thread. - Custom TLS contexts are refcounted. The cache holds one ref and each request holds one. <details><summary>Notes</summary> This is the part of oven-sh#31788 that main still needs. oven-sh#31788 was closed as stale, and all five of its `src/` files conflict with main now. - Its first defect, `abort_by_http_id` with no ref on the session, is fixed on main: since oven-sh#37870 every entry point goes through `ClientSession::enter`, which holds a `RefPtr`. The abort test from oven-sh#31788 passes on main. It is included here because main has no test that evicts a TLS context. - Its third part, a `ctx` field on `PendingConnect`, has no reproduction and is not included. ASAN report on main, trimmed: ``` ERROR: AddressSanitizer: heap-use-after-free READ of size 1 thread T2 (HTTP Client) #0 <State as PartialEq>::eq src/http/h2_client/Stream.rs:104 oven-sh#2 ClientSession::handle_data src/http/h2_client/ClientSession.rs:848 oven-sh#4 ClientSession::enter src/http/h2_client/ClientSession.rs:242 oven-sh#6 Handler<true>::on_data src/http/HTTPContext.rs:1386 freed by thread T2 (HTTP Client) here: oven-sh#12 drop_stream src/http/h2_client/ClientSession.rs:211 oven-sh#13 ClientSession::fail_streams src/http/h2_client/ClientSession.rs:943 ``` The two tests share one helper. The child holds a stream open on a context made by `serverName`, runs 61 more TLS configs to evict it, writes `evicted` to stderr, and then ends the held request: one test aborts it, the other has the server send `RST_STREAM(CANCEL)`. Both check that a later `fetch()` still works. They run only under ASAN and carry a 30 s timeout, because each child makes 62 TLS handshakes and a regression needs time to print its report. Without the longer timeout the failing run on main shows a timeout at 5 s, not the report. The tests are serial on purpose: each one fills the context cache, which would evict the context that a concurrent test depends on. One older test in the same file changed: `concurrent requests multiplex on one h2 session`. Its server held each stream for 100 ms and the test expected 8 open at once. On a loaded machine it saw 7 (2 of 9 runs of the file). The server now answers when the eighth stream is open, so the test uses no timer. After the change the file passed 4 of 4 runs on the same loaded machine. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/web/fetch/fetch-http2-client.test.ts" bun test v1.4.3 (b993710) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [1309.07ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [1215.67ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [1497.63ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body streams as raw DATA frames [730.67ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [2353.52ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST: request body is delivered as DATA frames [2514.64ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body larger than initial send window [927.77 ... (truncated) release without fix: 2 skipped bun test v1.4.3-canary.1 (f92be71) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [110.34ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [115.04ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response trailers are consumed without breaking the body [99.30ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > connection-specific request headers are stripped before HPACK [111.10ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > cold-start: parallel requests coalesce onto one TLS connect [135.66ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [147.00ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [160.06ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > keep-alive: sequential requests reuse one h2 session [139.51ms] (pass) fetch() over H ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/web/fetch/fetch-http2-client.test.ts" bun test v1.4.3 (b993710) test/js/web/fetch/fetch-http2-client.test.ts: (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > GET: status, headers and body round-trip [1532.08ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > response body larger than one DATA frame [1265.37ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST: request body is delivered as DATA frames [1731.76ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > POST with ReadableStream body streams as raw DATA frames [501.02ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > gzip content-encoding is decompressed [1802.42ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent requests multiplex on one h2 session [1743.84ms] (pass) fetch() over HTTP/2 (BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT) > concurrent ReadableStream uploads route each chunk to its own stream ... (truncated) release with fix: 2 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 710ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[92m Compiling�[0m bun_clap v0.0.0 (/workspace/bun/src/clap) �[1m�[92m Compiling�[0m bu ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/http/h2_client/ClientSession.rs | 12 ++- test/js/web/fetch/fetch-http2-client.test.ts | 118 ++++++++++++++++++++++++--- 2 files changed, 117 insertions(+), 13 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/http/h2_client/ClientSession.rs 2 1 24 test/js/web/fetch/fetch-http2-client.test.ts 1 2 24 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
bun run rust:miri, tearing down an h2 session or a proxy tunnel is reported as undefined behaviour: "deallocation through is forbidden ... the strongly protected tag disallows deallocations", pointing at the method's own receiver. The reduction in fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703 is this shape, and the ASAN use-after-free trace in http: fix use-after-free when aborting an h2 request after its custom TLS context is evicted #31788 is the same shape observed at runtime, inabort_by_http_id.bun_http(h2on_closeandmaybe_release,ProxyTunnel::detach_and_deref, h3detach) go through the method's&mut self. For the h2 and tunnel sites that release is normally the last one, so the object is freed while a reference argument to it is still live, which is UB whether or not the reference is used again.adoptandabort_by_http_id, held no ref of their own while their body could tear the session down;adoptthen read a field of the freed session. Theadoptcase was not triggered deterministically.Fix
createreturned) and runs through one wrapper: take a guard from that pointer, run the former&mut selfbody, then release the socket ref the body gave up (recorded in a flag) and the guard's ref, both after the borrow has ended.receiveandon_writablebuild their guard from the client's handle pointer;detach_and_derefis deleted (startnow builds the TLS wrapper before allocating the tunnel, and the pool fallback releases through the handle it was given);adoptmoves the pool's handle into the client instead of re-deriving one from the receiver.detach's release is never the last one (the connection's own ref outlives every stream), so it keeps&mut self, says why, and releases through the stream's backref.mainat the five receiver-release sites and passes on this branch; the existing h2, h3 and proxy suites pass on an ASAN debug build; clippy and fmt are clean. The lint is the only test that fails without the change.Background
ClientSessionandProxyTunnelcarry their ownref_count;deref(ptr)decrements it and frees the allocation at zero. Each holder (socket ext slot, context registry, keep-alive pool,HTTPClient.proxy_tunnel) owns one count, and hand-offs between holders move a count rather than bump it.HTTPContext.active_h2_sessionslists it so later requests can multiplex onto it, and the keep-alive pool holds it while it is parked with no streams.&mut Targument is protected for the whole call, so freeing the pointee during the call is UB even if the argument is never touched again. This is why the fix moves the free to after the body returns instead of avoiding later uses ofself.bun_ptr::ThisPtr/ScopedRef: a copyable raw handle to a refcounted object, and an RAII guard that bumps on construction and releases on drop.SessionPtrin this PR isThisPtr<ClientSession>.RefPtr(tunnels): the owning handle stored by the client or the pool;RefPtr::deref()releases the count that handle owns, so releasing through it is releasing through the holder's pointer.Original description
Problem
Four intrusive-refcount releases in
bun_httpwent through the method's own receiver,&mut selfcoerced to*mut Selfat the call:(
grep -rnP '\bderef\(\s*self\s*\)' srcat 9a543cc; the twoSubprocessPipeReader.rshits are being converted in their own PR.)For the h2 and tunnel sites the release is normally the last one. An h2 session is held by the socket ext slot, the context registry and (while parked) the pool;
on_closeleaves the registry and then releases the slot's ref, andmaybe_releasedoes the same on a connection it cannot pool, so the session is freed inside a method that still has a&mut selfargument pointing at it.detach_and_derefwas reached fromHTTPContext::release_socketwith the only remaining ref to the tunnel. Freeing an allocation while a reference argument to it is live is rejected by both aliasing models regardless of whether the reference is used again (Tree Borrows, the modelbun run rust:miriuses: "deallocation through is forbidden ... the strongly protected tag disallows deallocations", pointing at the receiver; Stacked Borrows: "deallocating while item [Unique] is strongly protected"). The reduction in #37703's description is this exact shape. The keep-alive guards had the same problem one step removed:ClientSession::ref_scope(&mut self)built aScopedReffrom the receiver and every socket event took one, so on thefail_allpaths the free happened at the guard's drop, still insideon_data/on_writable/resume_receive_by_http_id;ProxyTunnel::receive/on_writablebuilt theirs fromNonNull::from(&mut *self), and when delivering the bytes completes or fails the request (which releases the client's ref, or hands it to a full pool) that guard's drop is the last release.Two entry points additionally held no ref at all while their body could reach one of these releases.
adopt: both callers inHTTPContext::connect(registry match and pool resume) hold nothing of their own, so when the first flush of the new stream fails,attach->fail_all->on_closefreed the session andadoptthen readself.encoder_poisonedfrom the freed allocation (needs a TLS write to fail synchronously during the adopt; I could not trigger it deterministically).abort_by_http_id: the ASAN trace in #31788 is this shape observed, a custom TLS context dropping re-entrantly from the aborted request's result callback,on_closefreeing the session at its guard drop whileabort_by_http_id's&mut selfwas live up the stack, and the tail ofabort_by_http_idthen running on freed memory. #31788 fixes that (and two unrelated defects) by givingabort_by_http_idaref_scope()guard of its own, which moves the free to that guard's drop, still inside the method; here every entry point gets the guard fromenter, taken from the holder's pointer, and the free happens after the body's borrow has ended. The two PRs overlap in that one line and are otherwise independent.The h3 site is different: the per-stream ref
detachreleases is provably never the last one (the connection's ref is released only byon_conn_close/fail_session, and both drainpendingthroughdetachfirst), so&mut selfis a sound receiver there. The function now says so, and releases through the pending entry's own backref so that every release in the crate goes through the pointer of the holder whose ref it is.Fix
h2 (
h2_client/ClientSession.rs,HTTPContext.rs,HTTPThread.rs,lib.rs): every entry point that can end with the session released takes aSessionPtr(bun_ptr::ThisPtr<ClientSession>, the pointer its holder has: socket ext tag, registry entry, pool entry, or whatcreatereturned) and goes throughClientSession::enter, which takes a guard from that pointer, runs the former&mut selfbody through a call-scoped reborrow, and after it returns releases the socket-ext ref if the body gave it up (asocket_ref_owedcell set byfail_streamsand bymaybe_release's close branch, where thederef(self)calls were) and then the guard's own ref, both through the holder's pointer. Inside a body no release can be the last one (the guard holds one), and the two that can be last now run with no reference to the session in existence. Converted entries:on_data,on_writable,on_close,adopt, the leader'sattach_leader,enqueue,abort_by_http_id,stream_body_by_http_id,resume_receive_by_http_id,drain_response_body_by_http_id; the bodies are private, so the type has no&mut selfpath to a release left.ActiveSocketExt::session_mutbecomessession() -> Option<SessionPtr>,HTTPThreadholds its ownref_guard()across the resume + drain pair (what itsref_scope()was for),connect()adopts after its registry scan has finished instead of from inside the loop thatmaybe_releaseswap-removes from, and the registry releases its ref through the entry it stored rather than whatever pointer the caller passed. Order of operations within each entry is unchanged; the releases happen at the same points relative to everything else, just after the body's borrow has ended.proxy tunnel (
ProxyTunnel.rs,HTTPContext.rs,lib.rs):receiveandon_writabletake the client's handle pointer (RefPtr::data) and build their guard from it, the same contract the SSL callbacks in the file already use forref_scope.detach_and_derefis deleted:start()now builds the SSL wrapper before allocating the tunnel, so its failure path has nothing to release, and the pool fallback inrelease_socketreleases through theRefPtrit was handed, asclose_proxy_tunnel,AsyncHTTPand the shutdown path already do.adopttakes the pool'sRefPtrand moves it into the client instead of re-deriving a handle from its receiver withRefPtr::from_raw(from_mut(&mut *self)), so the ref the client eventually releases is the one the pool held.h3 (
h3_client/ClientSession.rs):detachdocuments why its release is never the last one and performs it through the stream'ssessionbackref.Test
test/internal/source-lints/self-receiver-deref.test.tsbansType::deref(self)(and the other raw-pointer release entry points of the refcount traits) andScopedRef/*RefGuard::new|adopt(self), checks its patterns against positive and negative spellings, and ratchets the twoSubprocessPipeReader.rssites at their exact count until their own conversion lands. Againstmainit reports exactlyand passes with this branch. It is deliberately limited to the bare receiver; the
ptr::from_mut(self)family is what #37703's lint covers, and the two compose.Verification
Debug (ASAN) build:
test/js/web/fetch/fetch-http2-client.test.ts,fetch-http2-adversarial.test.ts,fetch-http2-leak.test.ts,fetch-proxy-connect-tunnel-split-envelope.test.ts,fetch-proxy-tls-intern-race.test.ts,fetch-http3-client.test.ts,fetch-http3-adversarial.test.ts,fetch-http3-cold-post.test.ts, andtest/js/bun/http/proxy.test.{ts,js}plus the sevenproxy-stress-*suites all pass (twoproxy.test.jsauth cases needNO_PROXYunset in my container because it listslocalhost; they pass with it unset, and fail identically on the released binary with it set).bun test test/internal/source-lints/passes;cargo clippy -p bun_httpandcargo fmt --checkare clean.Not touched here, noted while reading: the pool's handle in
maybe_release(NonNull::from(&mut *self)) and the h2Stream/h3Streambackrefs are still pointers derived from a receiver, but none of them is a release site, and the re-entrantHTTPContextborrows described onunregister_h2_raware a separate problem.