http: remove lifetime-launder workarounds in client body/buffer and RareData socket-group paths - #35373
Conversation
- HTTPClient::to_result() now returns the result with body=None so it is
fully owned ('static) and can be held across the state.reset() that
follows; callers attach the caller-owned body buffer afterwards. This
replaces the detach_lifetime() at dispatch_result_and_reset (which
kept a live &'static mut to the body while state.reset() wrote through
the same allocation) and collapses the 40-line tuple-destructure in
both send_progress_update_* variants.
- handle_on_data_headers mem::take()s response_message_buffer into a
local so to_read is a plain &[u8] borrow of either incoming_data or
that local, both disjoint from &mut self. The short-read paths move
the buffer back; every other path drops it after clone_metadata() has
deep-copied the parsed header slices.
- RareData::*_group accessors take the uws loop pointer directly instead
of &VirtualMachine. Every caller reaches &mut RareData through
vm.rare_data(), which already holds &mut VirtualMachine; requiring a
second &VirtualMachine just to read vm.uws_loop() forced a raw-pointer
split-borrow at every call site. The loop pointer is read before
rare_data() is borrowed, so no aliasing.
Net: 10 files, -152 lines, 7 unsafe blocks removed.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
Comment |
|
Updated 3:42 AM PT - Jul 24th, 2026
✅ @robobun, your commit cbda8de431cc957bb9554079f25f9222c567c40c passed in 🧪 To try this PR locally: bunx bun-pr 35373That installs a local version of the PR into your bun-35373 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…peguard via BackRef Stacked on #35373, which retargets the RareData *_group accessors to take *mut uws::Loop. This PR finishes the rare_data/field-alias borrowck cleanup cluster (three sites #35373 doesn't cover). close_all_socket_groups was a &mut self method on RareData that didn't use self (let _ = self;): it walks the per-VM uSockets loop's linked group list, not RareData's embedded fields. Move it to VirtualMachine as a &self method so global_exit and WebWorker::shutdown call it directly instead of re-deriving &VirtualMachine through core::ptr::from_ref(self) while holding &mut rare_data. hot_reloader::on_file_update held the Watcher as a *mut so self could be reborrowed in the loop body and the flush_evictions scopeguard didn't pin a &mut across it. The reloader already stores a BackRef<Ctx> (Copy); have the scopeguard capture that and reach the Watcher on drop, and reborrow via self.get_context() at the two remove_at_index sites. Same BACKREF unsafe obligation, one audited site instead of three.
|
Heads-up: the trait doc on Also: #35376 is now stacked on this branch and covers the three remaining C4-cluster sites that don't depend on the |
…_loop reads - start_proxy_handshake: the mem::take is now a no-op (the sole caller already moved response_message_buffer out); replace it with a debug_assert on the empty buffer and keep only the must-not-touch-self-after-start note. - handle_response_body_from_single_packet / handle_response_body_chunked_encoding_from_single_packet: the response_message_buffer.owns() branches are now unreachable from every caller since handle_on_data_headers holds the buffer in a local; drop them (removes one more unsafe block, the provenance re-derivation in the chunked path). - socket_body.rs: hoist uws_loop() before rare_data() so it is read before the &mut VirtualMachine borrow, matching every other call site and the rare_data.rs doc comment. - WebSocketUpgradeClient: rebind from bun_vm() locally so the line-244 vm: &mut VirtualMachine does not stay live across the vm_ptr deref at poll_ref.ref(). - Drop stale trait doc on VirtualMachineSqlExt::postgres_socket_group.
These pin the behaviour of the three buffer paths the refactor touches: the short-read put-back (byte-by-byte header delivery), 1xx interim responses consumed from the accumulated buffer, and a chunked body arriving in the same read as the buffered header tail (now decoded via the scratch copy). Refactor only, so these pass on main too.
|
This is a refactor with no observable behaviour change (it removes eight Verification on the PR head (5007863): Ready for review. |
There was a problem hiding this comment.
My earlier findings (stale trait doc in sql_jsc/jsc.rs, dead envelope_buf in start_proxy_handshake) are both addressed at HEAD, and no new issues surfaced this run. Deferring to a human for the handle_on_data_headers rewrite and the removed owns() branches — the reasoning is sound but this is core HTTP client parsing / buffer-lifetime code and worth a maintainer's eyes.
What was reviewed:
to_result()→'staticwithbody = None: verified all three callers (dispatch_result_and_reset, bothsend_progress_update_*) attachbodyafterstate.reset(); no other callers exist.handle_on_data_headersbuffer take/put-back: traced every exit —short_read!and the empty-1xx path restorebuffer; terminal/body paths let it drop afterclone_metadata()deep-copies. Confirmed_from_single_packetvariants are only reached viais_only_buffer=true/ the ≤16 KiB gate, so the removedowns()branches are provably dead.RareData::*_group(loop_)signature change: checked all nine call sites readvm.uws_loop()beforerare_data()borrows, including the twosocket_body.rssites the description called out.
Extended reasoning...
Overview
Borrowck-audit refactor across 10 source files, net -195 lines. Three independent changes: (1) HTTPClient::to_result() now returns a fully-owned HTTPClientResult<'static> with body = None, and each caller attaches the body from state.body_out_str after state.reset() — replacing the prior detach_lifetime() launder and two 40-line destructure/rebuild blocks; (2) handle_on_data_headers now mem::takes response_message_buffer into a local so to_read is a plain &[u8] disjoint from &mut self, which makes three downstream response_message_buffer.owns(incoming_data) branches unreachable and they are removed; (3) RareData::*_group accessors take *mut uws::Loop directly instead of &VirtualMachine, eliminating the raw-pointer split-borrow at nine call sites (websocket client/upgrade, spawn IPC, test-parallel channel, Bun.connect, valkey, postgres/mysql).
Security risks
None identified. This is a lifetime/borrowck reshape with no change to what bytes are parsed, validated, or trusted. The removed unsafe blocks were provenance/lifetime workarounds, not validation. If anything the change is a net safety improvement: the acknowledged aliased-&mut in dispatch_result_and_reset (where state.reset() wrote through the same allocation as a live &'static mut) is gone, and the removed in-place chunked-decode branch had a hand-rolled provenance re-derivation.
Level of scrutiny
High. handle_on_data_headers is the HTTP/1.1 response-header state machine for every fetch() — the split-read accumulation, 1xx interim handling, proxy-tunnel handoff, and body dispatch all flow through it. The refactor changes when the accumulation buffer is dropped and what to_read borrows, and removes three downstream branches on the claim they're now unreachable. I traced each removed branch to confirm unreachability (the _from_single_packet variants are gated on is_only_buffer=true / len <= 16 KiB from the sole dispatcher, and response_message_buffer is empty at every dispatch point after the take), but this is exactly the kind of invariant a maintainer familiar with the H2/H3/proxy-tunnel entry points should confirm.
Other factors
My two earlier inline findings (stale VirtualMachineSqlExt trait doc; dead envelope_buf take in start_proxy_handshake) were both addressed in 44c420d / 5cd1a1b and are verified gone at HEAD. Three new tests in fetch-proxy-connect-tunnel-split-envelope.test.ts pin the split-read paths (byte-by-byte with 100 Continue, multi-1xx across reads, chunked body in the buffered header tail). The PR description lists a broad verification pass (proxy, HTTP/2, websocket, socket suites) and rust:check-all across all targets. This is a well-executed refactor; deferring only because the guidelines say not to auto-approve complex changes to critical paths.
|
Two corrections to the framing, plus a follow-up note: "No observable behaviour change" isn't quite true. Deleting the The tests are for the deleted code paths, not for the borrowck reshape — the body should say so instead of "there is no fail-before test." Concretely, the buffer take/put-back made three things unreachable and the PR deletes them; the tests pin those:
The other two new tests exercise moved code ( Follow-up, not this PR: for non-compressed chunked bodies |
handle_on_data_headers now moves the response_message_buffer into a local, so a pending_response: Option<Response<'static>> stored in InternalState would dangle once this function returned. Instead of documenting that every path either calls clone_metadata (which deep-copies) or never reads the field again, carry the parsed response as a local with its real lifetime and pass it into clone_metadata by reference: - clone_metadata(&mut self, &Response<'_>) deep-copies the argument; the compiler now checks the backing buffer outlives the copy. - apply_multiplexed_headers returns (HeaderResult, Response<'h>) borrowing the headers slice; the h2/h3 callers pass it straight to clone_metadata and no longer rely on a 'call clone_metadata before freeing' contract. - handle_on_data_headers keeps the parsed response as the 1xx loop's break value (shared_resp hoisted out of the loop so the borrow escapes). - Response::clone<'out> decouples the output lifetime from &self so the deep copy can be 'static while the input borrows a local. - InternalState.pending_response and picohttp::Response::detach_lifetime are deleted (no remaining callers). - Add a content-length single-packet test alongside the chunked one.
|
Body updated per the second comment: called out the one extra Filing the |
|
CI build 79458: the diff is green. The four reds are all marked flaky (passed on retry) and none touch this diff: All of the fetch/proxy/h2/h3/websocket lanes that exercise the changed code passed. |
…peguard via BackRef Stacked on #35373, which retargets the RareData *_group accessors to take *mut uws::Loop. This PR finishes the rare_data/field-alias borrowck cleanup cluster (three sites #35373 doesn't cover). close_all_socket_groups was a &mut self method on RareData that didn't use self (let _ = self;): it walks the per-VM uSockets loop's linked group list, not RareData's embedded fields. Move it to VirtualMachine as a &self method so global_exit and WebWorker::shutdown call it directly instead of re-deriving &VirtualMachine through core::ptr::from_ref(self) while holding &mut rare_data. hot_reloader::on_file_update held the Watcher as a *mut so self could be reborrowed in the loop body and the flush_evictions scopeguard didn't pin a &mut across it. The reloader already stores a BackRef<Ctx> (Copy); have the scopeguard capture that and reach the Watcher on drop, and reborrow via self.get_context() at the two remove_at_index sites. Same BACKREF unsafe obligation, one audited site instead of three.
…peguard via BackRef Stacked on #35373, which retargets the RareData *_group accessors to take *mut uws::Loop. This PR finishes the rare_data/field-alias borrowck cleanup cluster (three sites #35373 doesn't cover). close_all_socket_groups was a &mut self method on RareData that didn't use self (let _ = self;): it walks the per-VM uSockets loop's linked group list, not RareData's embedded fields. Move it to VirtualMachine as a &self method so global_exit and WebWorker::shutdown call it directly instead of re-deriving &VirtualMachine through core::ptr::from_ref(self) while holding &mut rare_data. hot_reloader::on_file_update held the Watcher as a *mut so self could be reborrowed in the loop body and the flush_evictions scopeguard didn't pin a &mut across it. The reloader already stores a BackRef<Ctx> (Copy); have the scopeguard capture that and reach the Watcher on drop, and reborrow via self.get_context() at the two remove_at_index sites. Same BACKREF unsafe obligation, one audited site instead of three.
…areData socket-group paths (#35373) Part of the borrowck-audit cleanup: replace lifetime-laundering workarounds in the HTTP client with ordinary borrows, and delete the code those workarounds were propping up. ## `HTTPClient::to_result()` / result dispatch `to_result()` previously returned an `HTTPClientResult<'_>` whose only lifetime-carrying field, `body: Option<&'a mut MutableString>`, was derived from `state.body_out_str` (a `NonNull` back-reference to caller-owned storage disjoint from `self`). The `&mut self` in the signature tied that borrow to `self`, so every caller either laundered it through `detach_lifetime()` or destructured every field into locals and rebuilt the struct. `dispatch_result_and_reset` did the former; its own comment noted that `state.reset()` wrote through `(*body_out_str).reset()` while `result.body` was a live `&'static mut` to the same allocation. `to_result()` now leaves `body = None`, so the result is fully owned and can be held across `state.reset()`. Each caller attaches the body from the `NonNull` afterwards. The two progress-update call sites drop their 40-line tuple-destructure blocks. ## `handle_on_data_headers` header-accumulation buffer `to_read` was held as a `bun_ptr::RawSlice` so subsequent `&mut self` calls would not trip the checker when it aliased `self.state.response_message_buffer`. `response_message_buffer` is now `mem::take`n into a local at the top of the function, so `to_read: &[u8]` borrows either `incoming_data` or that local, both disjoint from `&mut self`. The short-read paths move the buffer back; every other path lets it drop after `clone_metadata` has deep-copied the parsed header slices. That makes three downstream `response_message_buffer` consumers unreachable, so they go in the same PR: - `start_proxy_handshake`'s `envelope_buf = mem::take(...)` / `drop(envelope_buf)` pair always took an empty default (the original #30381 fix is now provided by the caller's take); replaced with a `debug_assert`. - `handle_response_body_from_single_packet`'s `if response_message_buffer.owns(incoming_data)` cleanup is never entered; removed. - `handle_response_body_chunked_encoding_from_single_packet`'s `owns()` in-place-decode branch (with its provenance-re-derivation `unsafe`) is never entered; the dispatcher already bounds `incoming_data.len()` to the 16 KiB scratch, so the function now copies into `small` unconditionally. One behaviour difference: a chunked body that arrives in the same `on_data` as the tail of a split header block (i.e. the headers spanned two or more reads) is now decoded out of the scratch copy rather than in place inside `response_message_buffer`. That is one extra `<= 16 KiB` `memcpy` per such response; the single-read case is unchanged. ## `clone_metadata` / `pending_response` On `main`, `state.pending_response: Option<Response<'static>>` was an erased borrow into `self.state.response_message_buffer`, safe because that buffer lived until `reset()`. Once the buffer moves into a local, the same erase would dangle when `handle_on_data_headers` returns, enforced only by a "caller MUST invoke `clone_metadata`" comment on `apply_multiplexed_headers`. That contract is now a signature: - `clone_metadata(&mut self, response: &picohttp::Response<'_>)` deep-copies the argument. - `apply_multiplexed_headers` returns `(HeaderResult, Response<'h>)` borrowing its `headers` slice; the h2/h3 callers pass it straight to `clone_metadata`. - `handle_on_data_headers` keeps the parsed response as the 1xx loop's break value (scratch header array hoisted so the borrow can escape the loop), then hands it to `handle_response_metadata` / `clone_metadata` without a lifetime erase. - `picohttp::Response::clone<'out>` decouples the output lifetime from `&self` so the deep copy can be `'static`. - `InternalState.pending_response` and `picohttp::Response::detach_lifetime` are deleted. ## `RareData::*_group` accessors `ws_client_group` / `ws_upgrade_group` / `spawn_ipc_group` / etc. took `&VirtualMachine` only to read `vm.uws_loop()`, but every caller reaches `&mut RareData` through `vm.rare_data()`, which already holds `&mut VirtualMachine`. That forced a raw-pointer split-borrow at six call sites: ```rust let vm_ptr: *mut _ = vm; unsafe { (*vm_ptr).rare_data().ws_client_group::<SSL>(&*vm_ptr) } ``` The accessors now take the `*mut uws::Loop` directly. The loop pointer is `Copy` and read before `rare_data()` is borrowed: ```rust let loop_ = vm.uws_loop(); vm.rare_data().ws_client_group::<SSL>(loop_) ``` All nine call sites now follow this order. ## Tests The new tests in `fetch-proxy-connect-tunnel-split-envelope.test.ts` pin the inputs that previously took the branches this PR deletes: | Deleted | Test | | --- | --- | | `owns()` in-place decode in `_chunked_encoding_from_single_packet` | "chunked body in the same read as the buffered header tail" | | `owns()` cleanup in `handle_response_body_from_single_packet` | "content-length body in the same read as the buffered header tail" | | `envelope_buf` take/drop in `start_proxy_handshake` | existing "split 200 envelope" (#30381) test in the same file | The 100 Continue / multi-1xx tests exercise the moved `short_read!` put-back and the 1xx `buffer.list.clear()` path as regression guards. ## Verification ``` bun run rust:check-all 10 ok, 0 failed bun bd test fetch-proxy-connect-tunnel-split-envelope.test.ts 5 pass bun bd test fetch-redirect.test.ts / fetch-http2-client.test.ts / fetch-http3-client.test.ts 129 pass bun bd test proxy.test.ts / proxy-stress-protocol.test.ts 164 pass bun bd test websocket.test.js / socket.test.ts no new failures vs main debug ``` ``` src/http/H2Client.rs | 4 +- src/http/InternalState.rs | 7 - src/http/ProxyTunnel.rs | 2 - src/http/h2_client/ClientSession.rs | 41 +- src/http/h3_client/ClientSession.rs | 15 +- src/http/lib.rs | 482 ++++++--------------- src/http_jsc/websocket_client.rs | 17 +- src/http_jsc/websocket_client/WebSocketUpgradeClient.rs | 13 +- src/jsc/VirtualMachine.rs | 15 +- src/jsc/rare_data.rs | 46 +- src/picohttp/lib.rs | 45 +- src/runtime/api/bun/js_bun_spawn_bindings.rs | 22 +- src/runtime/cli/test/parallel/Channel.rs | 10 +- src/runtime/socket/socket_body.rs | 20 +- src/runtime/valkey_jsc/js_valkey.rs | 25 +- src/sql_jsc/jsc.rs | 18 +- test/js/web/fetch/fetch-proxy-connect-tunnel-split-envelope.test.ts | 80 +++- 17 files changed, 329 insertions(+), 533 deletions(-) ``` Fixes #30767 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
Part of the borrowck-audit cleanup: replace lifetime-laundering workarounds in the HTTP client with ordinary borrows, and delete the code those workarounds were propping up.
HTTPClient::to_result()/ result dispatchto_result()previously returned anHTTPClientResult<'_>whose only lifetime-carrying field,body: Option<&'a mut MutableString>, was derived fromstate.body_out_str(aNonNullback-reference to caller-owned storage disjoint fromself). The&mut selfin the signature tied that borrow toself, so every caller either laundered it throughdetach_lifetime()or destructured every field into locals and rebuilt the struct.dispatch_result_and_resetdid the former; its own comment noted thatstate.reset()wrote through(*body_out_str).reset()whileresult.bodywas a live&'static mutto the same allocation.to_result()now leavesbody = None, so the result is fully owned and can be held acrossstate.reset(). Each caller attaches the body from theNonNullafterwards. The two progress-update call sites drop their 40-line tuple-destructure blocks.handle_on_data_headersheader-accumulation bufferto_readwas held as abun_ptr::RawSliceso subsequent&mut selfcalls would not trip the checker when it aliasedself.state.response_message_buffer.response_message_bufferis nowmem::taken into a local at the top of the function, soto_read: &[u8]borrows eitherincoming_dataor that local, both disjoint from&mut self. The short-read paths move the buffer back; every other path lets it drop afterclone_metadatahas deep-copied the parsed header slices. That makes three downstreamresponse_message_bufferconsumers unreachable, so they go in the same PR:start_proxy_handshake'senvelope_buf = mem::take(...)/drop(envelope_buf)pair always took an empty default (the original Bun fetch + HTTPS-over-CONNECT-proxy: raw upstream HTTP/1.1 leaks into response.body #30381 fix is now provided by the caller's take); replaced with adebug_assert.handle_response_body_from_single_packet'sif response_message_buffer.owns(incoming_data)cleanup is never entered; removed.handle_response_body_chunked_encoding_from_single_packet'sowns()in-place-decode branch (with its provenance-re-derivationunsafe) is never entered; the dispatcher already boundsincoming_data.len()to the 16 KiB scratch, so the function now copies intosmallunconditionally.One behaviour difference: a chunked body that arrives in the same
on_dataas the tail of a split header block (i.e. the headers spanned two or more reads) is now decoded out of the scratch copy rather than in place insideresponse_message_buffer. That is one extra<= 16 KiBmemcpyper such response; the single-read case is unchanged.clone_metadata/pending_responseOn
main,state.pending_response: Option<Response<'static>>was an erased borrow intoself.state.response_message_buffer, safe because that buffer lived untilreset(). Once the buffer moves into a local, the same erase would dangle whenhandle_on_data_headersreturns, enforced only by a "caller MUST invokeclone_metadata" comment onapply_multiplexed_headers. That contract is now a signature:clone_metadata(&mut self, response: &picohttp::Response<'_>)deep-copies the argument.apply_multiplexed_headersreturns(HeaderResult, Response<'h>)borrowing itsheadersslice; the h2/h3 callers pass it straight toclone_metadata.handle_on_data_headerskeeps the parsed response as the 1xx loop's break value (scratch header array hoisted so the borrow can escape the loop), then hands it tohandle_response_metadata/clone_metadatawithout a lifetime erase.picohttp::Response::clone<'out>decouples the output lifetime from&selfso the deep copy can be'static.InternalState.pending_responseandpicohttp::Response::detach_lifetimeare deleted.RareData::*_groupaccessorsws_client_group/ws_upgrade_group/spawn_ipc_group/ etc. took&VirtualMachineonly to readvm.uws_loop(), but every caller reaches&mut RareDatathroughvm.rare_data(), which already holds&mut VirtualMachine. That forced a raw-pointer split-borrow at six call sites:The accessors now take the
*mut uws::Loopdirectly. The loop pointer isCopyand read beforerare_data()is borrowed:All nine call sites now follow this order.
Tests
The new tests in
fetch-proxy-connect-tunnel-split-envelope.test.tspin the inputs that previously took the branches this PR deletes:owns()in-place decode in_chunked_encoding_from_single_packetowns()cleanup inhandle_response_body_from_single_packetenvelope_buftake/drop instart_proxy_handshakeThe 100 Continue / multi-1xx tests exercise the moved
short_read!put-back and the 1xxbuffer.list.clear()path as regression guards.Verification
Fixes #30767