Repository navigation
HTTP client: remove unsafe from src/http (h1/h2/h3 client, proxy tunnel, HTTP thread, keep-alive pool) - #40385
Jarred-Sumner wants to merge 13 commits into
Conversation
|
Updated 3:17 PM PT - Sep 6th, 2026
❌ @Jarred-Sumner, your commit 95995c5 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 40385That installs a local version of the PR into your bun-40385 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
9b4c2ef to
8da6a57
Compare
54abeb9 to
86f9d13
Compare
There was a problem hiding this comment.
This pass found no issues — the earlier rounds' fixes (RequestCell::arm() restoring result_callback/OwnerConfig on re-schedule, the dead on_data tunnel branch, the stale field docs, and the retry_or_fail → retry_or_fail_busy delegation) all check out at the current tip. Given the scope — a full ownership rework of the h1/h2/h3 client, proxy tunnel, HTTP thread, and keep-alive pool across 82 files — a human review is still the right call before merge.
Also examined and ruled out: release_in_flight_for_exit dropping RequestCell without releasing proxy_tunnel/custom_ssl_ctx — those RefPtr fields drop with the box, and the loop never ticks again so the socket ext aliasing noted in the comment is moot.
Extended reasoning...
Overview
This PR is the fetch-client-core instalment of the zero-unsafe programme (stacked on #40202). It rewrites the request ownership model — AsyncHTTP now carries a Box<RequestCell> that moves to the HTTP thread and back instead of the old ptr::read bitwise-copy — and converts every src/http/ file (h1/h2/h3 client sessions, ProxyTunnel, HTTPThread, HTTPContext, keep-alive pool, TLS session cache, lshpack) to &self + Cell/RefCell/BackRef/RefPtr. New primitives land in bun_core (heap::new_with, SecretCString, FfiSliceMut), boringssl_sys (SslSession, configure_client), bun_uws_sys (LoopWaker, SocketGroupCell, ssl_session_sink), and a new bun_lshpack_sys crate. 82 files, ~900 KB diff.
Security risks
The change touches TLS handshake configuration, session-resumption caching, rejectUnauthorized propagation across pooled sockets and h2 sessions, proxy CONNECT tunneling, and the checkServerIdentity park/resume path. The PR description calls out one intended tightening (UnsupportedProxyProtocol now also enforced on the custom-TLS cache-hit path) and several pre-existing issues fixed in passing. The bug-hunting pass did not surface new security regressions, but the amount of TLS-adjacent surface rewritten here is exactly the class REVIEW.md flags for careful human review.
Level of scrutiny
High. This is production-critical, memory-safety-sensitive code (the HTTP client every fetch(), bun install, and S3 request goes through), with a wholesale ownership-model change and intentional behaviour deltas listed in the description. It is well outside the "simple, mechanical, obvious" bar for auto-approval.
Other factors
Three prior automated review rounds each found issues (two 🔴 retry-path regressions, then dead code and stale docs, then a duplication) and all were addressed with follow-up commits (86f9d13, c7d3f63, 5123b3d). This fourth pass is clean; the one candidate raised and refuted was whether release_in_flight_for_exit leaks RefPtr fields when dropping in-flight RequestCells at process exit — it does not (Drop on RefPtr<ProxyTunnel>/RefPtr<HTTPContext> releases them, and the thread parks forever afterwards so nothing dereferences the socket ext). The PR carries extensive test evidence (84 test files under debug+ASAN, bun-install-retry.test.ts 10/10 covering the previously-broken retry path, hand-driven stress scenarios) and a −2.0 % keep-alive perf claim, but the design decisions (deferred completion flush, OwnerConfig capture semantics, h3 DNS ticket indirection) warrant a maintainer's sign-off.
c6d0dbf to
8eeaf3b
Compare
5123b3d to
6b50eea
Compare
There was a problem hiding this comment.
Beyond the inline nit, I also checked the MaybeUninit::new(AsyncHTTP::init(..)) overwrites in NetworkTask::for_manifest/for_tarball for leaking a parked Box<RequestCell> on retry — retries re-schedule() the existing AsyncHTTP rather than re-calling for_*, so the overwritten slot never holds a handed-back cell. Also checked h3 ClientSession::close_with for double-decrementing LIVE_SESSIONS — every caller gates on self.closed first, and closed.set(true) is the first line.
Extended reasoning...
This run's inline finding is a nit (dead unsafe lifetime erasure in run_command.rs whose SAFETY comment now points at a signature this PR changed). Two further candidates on the newly-pushed retry/ownership rework were investigated and ruled out: the NetworkTask MaybeUninit overwrite path is only reached on fresh hive slots (retries go through schedule() on the existing AsyncHTTP, not a fresh for_manifest/for_tarball), and h3 close_with's LIVE_SESSIONS decrement is guarded by closed.set(true) at entry with all call sites checking closed first. Noting these here since both touch the retry/teardown semantics that earlier review rounds flagged and that commits 4c49315 and 6b50eea reworked.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/cli/run_command.rs— nit:do_preconnect'sunsafe { from_raw_parts(...) }lifetime erasure is now dead — this PR changedasync_http::preconnect(url: URL<'static>)topreconnect(url: &URL<'_>)(AsyncHTTP.rs:360, called at line 877), andParsedURL::from_urlcopies the href, so the'staticrequirement the SAFETY comment names no longer exists. Per REVIEW.md "Delete dead code in the same PR that makes it dead" and the PR's own claim of "No new unsafe in src/runtime/** (several removed there)", this leftover should be removed andurl_strused directly.Extended reasoning...
No runtime failure — the erasure is a no-op on process-lifetime CLI argv storage. The cost is an unnecessary
unsafeblock with a stale justification ("soURL<'static>(whichAsyncHTTP::preconnectrequires)") pointing at a signature this PR changed, left in a diff whose stated purpose is eliminating exactly this class of lifetime-erasure unsafe. Fix: delete lines 847-852 and passurl_str(the&Box<[u8]>loop variable, which derefs to&[u8]) directly toURL::parse; the resultingURL<'_>is accepted by the newpreconnect(&url)and every intermediate check (is_http,hostname.is_empty,has_valid_port).Verification: nit — The
unsafelifetime erasure is now dead and its SAFETY comment is stale. Base branchsrc/http/AsyncHTTP.rs:388:pub fn preconnect(url: URL<'static>)— required'static. HEADsrc/http/AsyncHTTP.rs:360-365:pub fn preconnect(url: &URL<'_>) { Box::new(PreparedPreconnect { url: bun_url::ParsedURL::from_url(url) }).start(); }— takes a borrowed&URL<'_>.src/url/lib.rs:417-422…
|
Re the outside-diff note on |
f9e3e61 to
436a9d3
Compare
964fd54 to
443fcd3
Compare
5e88e58 to
95995c5
Compare
…l) to zero unsafe The HTTP thread no longer bit-copies the caller's AsyncHTTP into a ThreadlocalAsyncHTTP and copies it back. Scheduling a request moves its HTTPClient into a heap RequestCell (one memcpy, on the scheduling thread); the HTTP thread takes the cell, addresses it everywhere through a BackRef (socket ext tag, h2/h3 streams, proxy tunnel, pending-connect waiters), and once the terminal result is produced hands the cell (client reset to the owner's configuration) back to the AsyncHTTP before running the result callback, so an owner may schedule the same request again. Terminal delivery runs right after the socket event that produced it, never while a request is still borrowed. Per-thread statics become ThreadState, reached from every socket tag; cross-thread queues live on a Sync HttpThread static. - HTTPContext/KeepAlivePool/PooledSocket are Cell-based with counted RefPtr holders; the socket ext word is a typed ActiveSocket tag. - ProxyTunnel: SSLWrapper<BackRef<ProxyTunnel>>; callbacks that arrive while the owning request is busy are queued and replayed by it. - h2 ClientSession / h3 ClientSession are &self + Cell/RefCell with typed ref slots (socket_ref/conn_ref/released_ref); streams are Rc. - h3 DNS-pending connects are tickets in ThreadState, not raw pointers through the resolver. - RequestUrl keeps pre-resolved component slices (and the cached port) over the caller's or an owned href. - session cache uses an SslSession RAII type and a typed session sink; SSLConfig owns SecretCString fields; lshpack moves to bun_lshpack_sys with a caller-buffer decode contract; sendfile/chunked-decode/BoringSSL calls go through small safe wrappers in their sys crates; bun_core::heap::new_with builds large values in place. - Callers (fetch, install, s3, CLI, run --image) read results from HTTPClientResult (now carrying `elapsed`) instead of a copied-back AsyncHTTP; RawResultCallback replaces the fn-pointer pair.
…gured The cell handed back after a terminal result kept the previous attempt's per-hop rewrites (redirect budget, flags, proxy, hostname, unix socket path) and lost its result callback, so an install retry delivered to nobody. The request's client now lives in its RequestCell from the start (AsyncHTTP::client_mut() configures it before scheduling); the first schedule takes the owner-set fields a run may rewrite, and every later one puts them back and sets the callback afresh. Per-attempt buffers are freed on the HTTP thread at hand-back. S3 tasks now drop their AsyncHTTP (and with it the cell) instead of clearing two fields of it. Completed cells are found by slot instead of a scan, and AsyncHTTP::init records the URL once.
… unreachable; trim three stale field docs
…d unix_socket_path and RefPtr drop-release onto the RequestCell model
…s; h2 SessionPtr doc names RefPtr::from_this
…xHttpHeaderSize()
… through the caller's client borrow
…ePool/RequestCell model Two lazily-allocated pools per context (TCP 64, unix 128) keyed by Transport, longest-idle eviction on a full pool via a per-context park sequence, ssl_config matched by content hash instead of a parked strong ref, unix TLS entries carry the verified hostname in target_hostname, connect_socket consults the pool, the custom-TLS-context path dials unix sockets too, and the TLS session cache key includes the socket path.
443fcd3 to
11fcb8a
Compare
#42900) ### Problem - An HTTP/3 `fetch()` whose QUIC connection dies before the response header can abort the process. ASan: `heap-use-after-free READ of size 8` in `HTTPClient::fail_from_h2` (`src/http/lib.rs:2108`), from `ClientSession::retry_or_fail` (`src/http/h3_client/ClientSession.rs:288`). Release builds panic: `fetch on the HTTP thread holds a ticket`. - The retry queues the request on a new session through `ClientContext::connect`. When no connection opens, connect fails that session with `PendingConnect::fail_session`, which fails every request queued on it. That dispatch frees the `AsyncHTTP` the client is part of. Then the retry fails the same client again. ### Fix - `connect` takes the request back off the session before it fails that session. A `false` return leaves the request on no session, so the caller is its only failure path, which is what the other two callers assume. - Correct because the session is one call old: the request `enqueue` just queued is its only entry, so `detach` leaves `fail_session` nothing to fail. The teardown, the registry removal and the session's last reference do not change. - The retried request keeps the error of the stream that closed. `start_` still reports `ConnectionRefused` for its own failed connect. - Verified: `test/js/web/fetch/fetch-http3-client.test.ts`, one new test (main aborts with an empty stdout). Also the three other `fetch-http3-*` suites, `serve-http3` and `serve-protocols`. ### Background - The h3 fetch client pools one QUIC connection per origin. `retry_or_fail` re-sends a stream that closed before any response header, once, on a fresh connection. - `ClientContext::connect` finds a pooled connection or opens one, and queues the request. `enqueue` binds a `Stream` to the request before the QUIC connect, because that stream has to exist when the handshake completes. - `HTTPClient::start_` sets `defer_terminal_dispatch_until_connecting_is_complete` before its own connect call, so a failure inside that frame is recorded and dispatched later. That flag is why the two initial connect sites survived the double failure. <details><summary>Notes</summary> **Fail-before.** With `src/` and `packages/` back on `55c11065f2`, the new test gives `exitCode: 1` and an empty stdout. That run, the passing run and the suites above were on `55c11065f2` plus this change, built with LLVM 21. The branch has since merged main, which needs LLVM 23 (#42851). The build environment used here does not have it, so on the merged tree only `cargo check` and `cargo clippy` for `bun_http` were run locally, and CI is the test run for it. The three commits that merge brought in touch none of the files involved. The ASan frames are the report above: ``` READ of size 8 at 0x... thread T4 (HTTP Client) #2 <bun_http::HTTPClient>::fail_from_h2 src/http/lib.rs:2108 #3 <ClientSession>::retry_or_fail src/http/h3_client/ClientSession.rs:288 #4 h3_client::callbacks::on_conn_close src/http/h3_client/callbacks.rs:151 freed by thread T4 (HTTP Client) here: #7 <AsyncHTTP>::on_async_http_callback_raw src/http/AsyncHTTP.rs:783 #10 <bun_http::HTTPClient>::fail_from_h2 src/http/lib.rs:2122 #11 <PendingConnect>::fail_session src/http/h3_client/PendingConnect.rs:149 #12 <ClientContext>::connect src/http/h3_client/ClientContext.rs:179 #13 <ClientSession>::retry_or_fail src/http/h3_client/ClientSession.rs:287 ``` A release build aborts as well, so the fault is not an ASan artifact: `on_async_http_callback_raw` resets the client's stage before the dealloc, so the once-only guard in `fail_from_h2` cannot stop the second dispatch. Making that guard survive the reset is a separate change. **How the test reaches it.** A connect to a resolved hostname probes each address with a throwaway UDP `connect(2)`, and gives up when no entry is reachable (`packages/bun-usockets/src/quic.c`, `us_quic_connect_result`). An `LD_PRELOAD` shim allows the first probe and refuses every later one, so the reconnect fails inside `connect`. `rejectUnauthorized` against the suite's self-signed certificate fails the handshake, which is what closes the stream before any header and starts the retry. `localhost` answers from `is_localhost_name` as `[::1, 127.0.0.1]` without the resolver, so no connect waits for DNS, and the shim refuses the IPv6 entry the way a host without an IPv6 route does, which pins both connects to the same address. Linux only, and only where a C compiler exists, like the DPLPMTUD shim test in `fetch-http3-syscall-fault.test.ts`. 5 runs, 5 passes, about 500 ms each on the debug ASan build. **Other ways to reach the same failure.** Any synchronous failure of the QUIC connect does it: a cached resolver error, an IP literal whose family the shared client endpoint cannot serve, `lsquic_engine_connect` returning NULL, or the shared client UDP endpoint dying on a hard `recvmsg` error and the poll registration for its replacement failing. The last one needs no resolver, so it reaches this path for an IP-literal origin too. One test is enough: all of them end in the same `return false`, and the endpoint-replacement route needs several iterations of a loop to line up. **Earlier shape.** The first version of this PR removed the retry's failure call instead, and documented `connect` as owning the request. Review pushed back: it left both `if !connect { self.fail(..) }` arms in `start_` dead, it made the bool unusable by every caller, and it set the opposite contract from #40385, which removes the same double failure from the callee side. This version fixes the callee, which also keeps the closed stream's error in the rejection instead of replacing it with `ECONNREFUSED`. **Scope.** `retry_or_fail` is also edited by #41564 (a retry budget) and #42579 (no replay of a non-idempotent request), and #40598 changes which pre-header closes retry. None of them touch this branch, so this applies on top of any of them, and #40385 keeps the same contract. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-http3-client.test.ts <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
What
Same programme as #40055 … #40383, applied to the fetch client core below the JS layer. Stacked on #40202 (base branch
claude/fetch-zero-unsafe; also carries #40210's twobun_ptrcommits). Retarget to main once #40202 lands.src/http/(excludingwebsocket_client/, which #40055 already did): every file → 0.lib.rs64,HTTPContext.rs25,HTTPThread.rs22,session_cache.rs18,AsyncHTTP.rs14,ProxyTunnel.rs12,lshpack.rs11 (→ newbun_lshpack_syscrate),ssl_config.rs9,ThreadSafeStreamBuffer.rs7,SendFile.rs3,h2_client/*13,h3_client/*28, and the singles — all → 0. No newunsafeinsrc/runtime/**orsrc/http_jsc/**(several removed there).ptr::reads the caller'sAsyncHTTPand copies state back.AsyncHTTP::schedule()moves itsHTTPClientinto aBox<RequestCell>(built in place) and parks it on the request; the HTTP thread takes the box;RequestRef = BackRef<RequestCell>is the socket ext tag / h2-h3 stream back-ref / tunnel owner / pending-connect waiter; a terminal result is stored on the cell andThreadState::flush_completions()(after every socket event / drain step / h3 callback) resets the client to the owner's configuration, parks the cell back on the owner, setshanded_back, then runs the callback — re-scheduling (install retries) reuses the cell, and no callback runs under a live&mut HTTPClient.ThreadState(HTTP-thread only,Cell/RefCell) replaces the&'static mutglobals; cross-thread queues live onstatic HTTP_THREAD: HttpThread(Guarded/atomics/OnceLock<LoopWaker>).SSLWrapper<BackRef<ProxyTunnel>>+ a deferredTunnelEventqueue. h2/h3 sessions are&self+Cell/RefCell,Rc<Stream>, typed ref slots; h3 DNS pending is an index ticket.RequestUrl= pre-resolved components + optional owned backing;HTTPResponseMetadataowns its headers;SSLConfigstrings areSecretCString; TLS session resumption viaboringssl_sys::SslSessionRAII +ssl_session_sink.bun_core::heap::new_with(in-placeBoxconstruction),FfiSliceMut,SecretCString;bun_uws_sys::{LoopWaker, SocketGroupCell, Socket::{ext_word, connect_group_tagged}, ssl_session_sink, quic::ClientHandler};MiniEventLoop::run_uws_loop_forever;boringssl_sys::SSL::{peer_leaf_certificate, is_init_finished, configure_client, alpn_selected, set_session},X509::to_der;bun_sys::{linux::sendfile_at, c::sendfile_plain};bun_url::{ParsedURL, URL::from_parts};bun_picohttp::decode_chunked;bun_lshpack_sys;HTTPClientResultCallback::{Raw, Handler(Arc), ThreadOwned};Bun__defaultMaxHttpHeaderSize()HOST_EXPORT.Parity deltas (intentional, small): terminal teardown happens at completion-flush right after the callback rather than just before; h3 same-thread DNS completion is observed one loop turn later; an h3 retry after a refused re-connect surfaces the original error;
UnsupportedProxyProtocolalso enforced on the custom-TLS cache-hit path; install/S3 retries start from pristine request config. Pre-existing issues fixed in passing:write_to_streamreleased the JS stream-buffer lock after a synchronous tunnelon_closecould have reset the request;on_handshakeexpect-panicked on a missingSSL*; h2encode_headerwrote into uninitialisedVeccapacity; h2give_up_socket_refcould overwrite a parked ref; h2attachcompress-failure didn't flush the preface; h3 connect-error path detached a freed stream.Perf (release,
taskset8 cores,perf stat -e instructions:uon the client process, interleaved ×8): 20k keep-alivefetch()against a local server — after the rebase onto #40478/#40391/#35886, −0.37 % vs the parent branch (7/8 rounds lower; an earlier base measured −2.0 %). 2k fresh-TLS and 200 × 1 MB concurrent: within noise.Retry path (install/S3 re-schedule the same
AsyncHTTP):RequestCell::arm()runs on everyschedule()— first time it captures the owner's config (flags, redirect budget,hostname,if_modified_since, ownership ofhttp_proxy/unix_socket_path), every time it resets the client to that config, rebuilds headers/url/method from theAsyncHTTP, and installs the result callback afresh. Per-attempt resources are still released on the HTTP thread at completion. Intended delta: a retry starts from the owner'sforced_protocol/disable_keepalive/redirect budget rather than the previous attempt's learned values.test/cli/install/bun-install-retry.test.ts10/10.Testing
Debug+ASAN,
--timeout 60000: 84 files acrosstest/js/web/fetch/*(incl. http2/http3/proxy/redirect/keepalive/leak/unix/upgrade),test/js/bun/http/{proxy*,fetch*,tls-keepalive},test/js/bun/s3/*,test/js/node/http/*, 445test/js/node/test/parallel/test-http*/https-*— green except known sandbox reds (fetch.test.ts root-permission ×4 + 150 msAbortSignal.timeout;fetch-tcp-stress/fetch-leak › URLSearchParamsdebug timeouts that pass on release;test-http-client-req-error-dont-double-fire.jsidentical under system bun).bun-install-registry.test.ts245/246. Hand-driven: 1000 keep-alive fetches on one socket; redirect ×20; gzip/br/zstd/deflate; chunked streaming upload; 500× abort mid-body + GC; self-signed reject /rejectUnauthorized:false/ CA /checkServerIdentity;NODE_TLS_REJECT_UNAUTHORIZED=0; HTTP + CONNECT proxy with tunnel reuse; server close mid-response/mid-chunk; Worker terminated mid-download ×5 — exit 0, no ASAN output. clippy clean;rust-check-allwindows-msvc + apple-darwin pass.