http: fix ProxyTunnel use-after-free when response completes mid-handleReading#30606
http: fix ProxyTunnel use-after-free when response completes mid-handleReading#30606robobun wants to merge 5 commits into
Conversation
When a proxied HTTPS response completes inside SSLWrapper.handleReading's triggerDataCallback, the HTTPClient (embedded in ThreadlocalAsyncHTTP) is freed synchronously by onAsyncHTTPCallback. If the same handleReading call then hits an SSL error/close_notify, triggerCloseCallback fires onClose with the now-freed *HTTPClient stored in handlers.ctx. Point handlers.ctx at the refcounted ProxyTunnel instead (kept alive across receive()/onWritable() via ref/deref) and look up the owning client through a nullable owner field that detachOwner()/detachSocket() clear. Callbacks early-return when there is no owner, so a pooled or detaching tunnel never dereferences freed memory.
|
Updated 1:56 AM PT - May 13th, 2026
❌ @robobun, your commit 80f7b5d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30606That installs a local version of the PR into your bun-30606 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughProxyTunnel callbacks now receive the tunnel and derive the HTTPClient from a nullable owner field cleared on detach to prevent stale-callback use-after-free. Lifecycle init, onWritable/tunnel liveness checks, and pooled-socket deinit docs updated. A regression fixture and test reproduce and assert the behavior under concurrent proxied HTTPS requests. ChangesProxyTunnel use-after-free safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts`:
- Around line 93-103: The test currently asserts exitCode before validating
stdout; change the order so you first await and parse stdout/stderr (use
proc.stdout.text(), proc.stderr.text(), proc.exited as before), then parse
stdout into lines and assert result.ok (the existing JSON parse and
expect(result.ok).toBeGreaterThan(0)) before asserting exitCode; finally, if
exitCode !== 0 log the stderr and then expect(exitCode).toBe(0). Update the
block that references stdout, stderr, exitCode, lines, and result to perform
stdout assertions first and exitCode assertion last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 346d71fc-f2fd-4d48-89af-4ed9bb21bf3f
📒 Files selected for processing (3)
src/http/HTTPContext.zigsrc/http/ProxyTunnel.zigtest/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| if (exitCode !== 0) { | ||
| console.error("Fixture stderr:", stderr); | ||
| } | ||
| expect(exitCode).toBe(0); | ||
|
|
||
| const lines = stdout.trim().split("\n"); | ||
| const result = JSON.parse(lines[lines.length - 1]); | ||
| expect(result.ok).toBeGreaterThan(0); | ||
| } finally { |
There was a problem hiding this comment.
Reorder subprocess assertions so exit code is asserted last.
On Line 98, exit code is asserted before stdout expectations. Move the stdout parsing/assertion first, then assert exit code last.
Suggested patch
- if (exitCode !== 0) {
- console.error("Fixture stderr:", stderr);
- }
- expect(exitCode).toBe(0);
-
const lines = stdout.trim().split("\n");
const result = JSON.parse(lines[lines.length - 1]);
expect(result.ok).toBeGreaterThan(0);
+ if (exitCode !== 0) {
+ console.error("Fixture stderr:", stderr);
+ }
+ expect(exitCode).toBe(0);As per coding guidelines, when spawning processes in tests, check stdout expectations before exit code expectations to provide more useful error messages on test failure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| if (exitCode !== 0) { | |
| console.error("Fixture stderr:", stderr); | |
| } | |
| expect(exitCode).toBe(0); | |
| const lines = stdout.trim().split("\n"); | |
| const result = JSON.parse(lines[lines.length - 1]); | |
| expect(result.ok).toBeGreaterThan(0); | |
| } finally { | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| const lines = stdout.trim().split("\n"); | |
| const result = JSON.parse(lines[lines.length - 1]); | |
| expect(result.ok).toBeGreaterThan(0); | |
| if (exitCode !== 0) { | |
| console.error("Fixture stderr:", stderr); | |
| } | |
| expect(exitCode).toBe(0); | |
| } finally { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts` around lines 93 -
103, The test currently asserts exitCode before validating stdout; change the
order so you first await and parse stdout/stderr (use proc.stdout.text(),
proc.stderr.text(), proc.exited as before), then parse stdout into lines and
assert result.ok (the existing JSON parse and
expect(result.ok).toBeGreaterThan(0)) before asserting exitCode; finally, if
exitCode !== 0 log the stderr and then expect(exitCode).toBe(0). Update the
block that references stdout, stderr, exitCode, lines, and result to perform
stdout assertions first and exitCode assertion last.
|
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}
| if (exitCode !== 0) { | ||
| console.error("Fixture stderr:", stderr); | ||
| } | ||
| expect(exitCode).toBe(0); | ||
|
|
||
| const lines = stdout.trim().split("\n"); | ||
| const result = JSON.parse(lines[lines.length - 1]); | ||
| expect(result.ok).toBeGreaterThan(0); |
There was a problem hiding this comment.
🟡 Per CLAUDE.md (line 128), tests that spawn processes should assert on stdout before exitCode so failures surface useful output. Here expect(exitCode).toBe(0) runs before stdout is parsed/asserted, so if the subprocess crashes you only see "expected 0, received 134" without stdout. Consider asserting on stdout (or at least logging it in the failure block) before the exitCode check.
Extended reasoning...
What the issue is
The repo's testing conventions explicitly state in CLAUDE.md:128:
When spawning processes, tests should
expect(stdout).toBe(...)BEFOREexpect(exitCode).toBe(0). This gives you a more useful error message on test failure.
And test/CLAUDE.md shows the canonical ordering: expect(stderr).toBe(""); expect(stdout).toBe(...); expect(exitCode).toBe(0);.
In test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts:95-102, the new test does the opposite:
if (exitCode !== 0) {
console.error("Fixture stderr:", stderr);
}
expect(exitCode).toBe(0);
const lines = stdout.trim().split("\n");
const result = JSON.parse(lines[lines.length - 1]);
expect(result.ok).toBeGreaterThan(0);The exitCode assertion comes first; the stdout parse/assert comes after.
How it manifests / why it matters
This test is specifically guarding against a UAF crash. If a regression reintroduces the bug:
- The subprocess aborts (e.g. ASAN
SIGABRT, exit code 134). - The test enters the
if (exitCode !== 0)block and logs stderr — good, the ASAN report likely lives there. expect(exitCode).toBe(0)fails and the test stops.- stdout is never logged or asserted. Anything the fixture wrote to stdout before crashing (e.g. partial JSON, debug output, the
{ok: N}line if the crash happened afterconsole.log) is silently discarded.
So while the stderr log provides partial mitigation, stdout is dropped on the floor, and the documented project convention is violated.
Step-by-step proof
- Suppose the fixture completes 3 of 5 rounds, prints something to stdout, then hits the UAF and aborts with exit code 134.
exitCode !== 0→console.error("Fixture stderr:", stderr)runs (stderr shown).expect(exitCode).toBe(0)throws → test fails with "expected 0, received 134".- Lines 100-102 never execute →
stdoutis never inspected or printed. - CI output: just the exit-code mismatch + stderr; no stdout.
Compare with the convention-compliant ordering: asserting on stdout first would surface the actual fixture output in the failure message (or, if stdout were also logged in the error block, it would be visible alongside stderr).
Why existing code doesn't prevent it
The console.error("Fixture stderr:", stderr) guard only covers stderr. There is no equivalent for stdout, and the early expect(exitCode) short-circuits before the stdout assertions on lines 100-102 can run.
Fix
Either reorder to match the convention:
const lines = stdout.trim().split("\n");
const result = JSON.parse(lines[lines.length - 1] || "{}");
expect(result.ok).toBeGreaterThan(0);
expect(exitCode).toBe(0);…or, more minimally, also log stdout in the failure block:
if (exitCode !== 0) {
console.error("Fixture stderr:", stderr);
console.error("Fixture stdout:", stdout);
}This is a test-convention nit — it doesn't affect the correctness of the test's pass/fail behavior, only the diagnosability of failures.
Replace the best-effort proxy/keepalive load test with a fixture that appends a malformed TLS record immediately after the HTTP-response record on the proxied connection. SSLWrapper.handleReading then sees app-data + SSL_ERROR_SSL in one call: triggerDataCallback completes the request (freeing the HTTPClient via the keepalive pool path, since received_ssl_shutdown stays false), and triggerCloseCallback fires on the freed pointer. On debug+ASAN this aborts with use-after-poison at ProxyTunnel.onClose pre-fix and exits cleanly with the owner-field fix. Also guard HTTPClient.onWritable: proxy.onWritable()'s deferred flush() can drive the same completion path, after which reading this.state.request_stage is a UAF. Check proxy.owner after the call and bail if the tunnel detached.
When SSL_read returns SSL_ERROR_SSL/SYSCALL in the same handleReading() that delivers a keepalive response, flags.fatal_error is set before triggerDataCallback but isShutdown() (used by tunnel_poolable) doesn't look at it — so the dead wrapper was pooled and the ownerless onClose silently returned. A subsequent adopt() would then try to write through a closed_notified wrapper and swallow error.ConnectionClosed, hanging until timeout. Check fatal_error alongside isShutdown().
| const lastLine = stdout.trim().split("\n").pop()!; | ||
| const result = JSON.parse(lastLine); | ||
| expect(result.ok).toBeGreaterThan(0); | ||
| }, 30_000); |
There was a problem hiding this comment.
🟡 nit: test/CLAUDE.md says "Do not set a timeout on tests. Bun already has timeouts." — though given this fixture does 4×32 proxied TLS handshakes the 30s is probably load-bearing on slow CI, so if you want to drop it consider trimming the round/batch counts instead.
Extended reasoning...
What this is
test/CLAUDE.md:120 states:
CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
The new test passes 30_000 as the second argument to test() at test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts:45, which is an explicit per-test timeout. That's a documented-convention violation, not a functional bug — the test passes/fails identically either way.
Step-by-step
test/CLAUDE.md:120documents the rule.- Line 45 of the new file ends the
test("...", async () => { ... }, 30_000);call with an explicit 30-second timeout literal. - There is no other mechanism in this file (no
jest.setTimeout, no harness flag) that would supply a timeout, so the30_000is the only thing overriding Bun's default.
Why the rule isn't a clean fit here
The fixture spawns a subprocess that performs 4 rounds × 32 = 128 proxied HTTPS requests. Each one is TCP connect → CONNECT → inner TLS handshake → request/response. On slow CI runners (Windows in particular) that can comfortably exceed Bun's default 5s test timeout, so simply deleting 30_000 would likely make the test flaky — i.e. the "fix" is worse than the violation.
This is also far from unique in the codebase: a grep shows 150+ test files passing a numeric second argument to test(), including the immediately-neighbouring fetch-leak.test.ts, fetch-proxy-tls-intern-race.test.ts, and fetch-redirect.test.ts. So enforcement of this rule is already inconsistent and the new file matches the local pattern rather than diverging from it.
Why flag it anyway
It's new code being added, the guideline is explicitly marked CRITICAL in the repo's own docs, and there's a straightforward alternative that satisfies both concerns: the iteration counts in the fixture are arbitrary load-generators, not correctness-bearing constants.
Suggested fix
Either leave the timeout (matching neighbours) and ignore this nit, or — if you'd prefer to follow test/CLAUDE.md — drop the 30_000 and reduce the fixture's workload so it fits the default timeout, e.g.:
for (let round = 0; round < 2; round++) {
for (let i = 0; i < 16; i++) {32 concurrent + 2 rounds is still plenty to trip the UAF under ASAN (the original repro is a single-request race; the batching just amortises scheduling jitter).
There was a problem hiding this comment.
Leaving the 30s timeout — matches the neighbouring fetch-proxy-tls-intern-race.test.ts and is load-bearing for 128 proxied TLS handshakes on slow CI. Trimming the batch count risks losing determinism on the UAF repro.
tunnel_poolable is a snapshot taken inside triggerDataCallback; a close_notify or SSL error arriving later in the same handleReading() (e.g. the buffer-fill flush at ssl_wrapper.zig:468 for a >=64KB body) lands after detachOwner() and the ownerless onClose no-ops, leaving a closed_notified/fatal_error wrapper in the pool. existingSocket() only validated the outer TCP socket, so the next matching request would adopt() a dead inner TLS session and hang on a swallowed error.ConnectionClosed. Check the wrapper's isShutdown()/fatal_error before handing a pooled tunnel back.
|
This crash signature (ProxyTunnel onClose UAF via handle_reading's trigger_close_callback after the response completes mid-read) is still present on current main in the shipping Rust implementation. The files this PR patches (src/http/ProxyTunnel.zig, src/http/http.zig, src/http/HTTPContext.zig) are the Zig porting references and are no longer compiled, so this change does not affect the built binary. #31959 fixes it in the Rust path (src/uws/lib.rs SSLWrapper::shutdown marks the wrapper closed_notified on the already-shut-down fast-shutdown early return) with a deterministic single-shot ASAN repro. This PR can probably be closed once that lands. |
…letes mid-read (#31959) [publish images] Fixes a use-after-free in the HTTP client's proxy tunnel close path (Sentry BUN-2VY8, ~10 events/day on Windows release builds; reproduces deterministically under ASAN on all platforms). ## Repro `fetch()` through an HTTP CONNECT proxy to an HTTPS origin, where the origin's final response bytes and its TLS `close_notify` reach the client in a single TCP batch (origin writes the response and immediately closes). The regression test builds exactly that: a local CONNECT proxy that holds origin-to-client bytes after the handshake and flushes session tickets + response + close_notify in one write. On an unfixed ASAN build: ``` ERROR: AddressSanitizer: heap-use-after-free READ of size 8 thread T11 (HTTP Client) #0 Option<RefPtr<ProxyTunnel>>::as_ref #1 bun_http::proxy_tunnel::on_close src/http/ProxyTunnel.rs:525 #2 SSLWrapper<*mut HTTPClient>::trigger_close_callback src/uws/lib.rs:802 #3 SSLWrapper<*mut HTTPClient>::handle_reading src/uws/lib.rs:1022 #4 SSLWrapper<*mut HTTPClient>::handle_traffic #5 SSLWrapper<*mut HTTPClient>::receive_data #6 ProxyTunnel::receive src/http/ProxyTunnel.rs:751 freed by: AsyncHTTP::on_async_http_callback_raw src/http/AsyncHTTP.rs:813 HTTPClient::send_progress_update_without_stage_check src/http/lib.rs:3793 ``` ## Cause 1. `handle_reading` processes the batch: `SSL_read` returns the body bytes, the next `SSL_read` hits `close_notify` (`SSL_ERROR_ZERO_RETURN`), which sets `received_ssl_shutdown` and `sent_ssl_shutdown` before flushing the already-decrypted bytes through the data callback. 2. The data callback completes the response. The done path runs `close_proxy_tunnel(true)` -> `ProxyTunnel::shutdown()` -> `SSLWrapper::shutdown(true)`, which hits the already-shut-down early return (`sent_ssl_shutdown || fatal_error`) and returns **without setting `closed_notified`**. The result callback then frees the `ThreadlocalAsyncHTTP` embedding the `HTTPClient`, the exact pointer stored in the wrapper's `handlers.ctx`. 3. Control returns to `handle_reading`. Its liveness guard (`ssl.is_none() || closed_notified()`) passes because neither is set, so `trigger_close_callback()` invokes `on_close(handlers.ctx)` on the freed client. When the allocation has been recycled, `on_close` can ref or close a different request's tunnel instead of faulting. ## Fix `src/uws/lib.rs`: when `SSLWrapper::shutdown(fast_shutdown=true)` takes the already-shut-down early return, fire `trigger_close_callback()` (idempotent via `closed_notified`) so the wrapper is marked closed before the owner detaches and frees `handlers.ctx`. A fast shutdown is a full teardown, and the normal fast-shutdown path already fires the close callback unconditionally; this only closes the gap where the SSL-level shutdown had already happened. Graceful `shutdown(false)` (node:tls half-close via UpgradedDuplex / WindowsNamedPipe) is unchanged, so reads after a sent `close_notify` keep working. ## Verification New test in `test/js/bun/http/proxy.test.ts` (`test.skipIf(!isASAN)`, the UAF is only deterministic under ASAN): fails on an unfixed ASAN debug build with the heap-use-after-free above, passes with the fix. Full `proxy.test.ts` (46 tests) plus `node-tls-connect`, `node-tls-upgrade`, `node-tls-duplex-close-throw-uaf`, `node-tls-socket-allow-half-open-option`, `node-tls-server`, `fetch-tls-cert`, and `node-https-checkServerIdentity` suites pass. ## Note on the asan-lane CI failure (#32144) The intermittent LeakSanitizer failure on the x64-asan shard (deferred napi finalizers parked on a never-drained cleanup-hook list at `bun test` exit) is being fixed in #32146, which carries the same `global_exit()` drain plus a hooks-only guard that skips pending `napi_wrap` finalizers on undrained-loop exits. A subset version of that fix was briefly on this branch (e59bc1d) but without the hooks-only guard it made `test/js/third_party/duckdb/duckdb-basic-usage.test.ts` SEGV at exit on the asan lane (build 62135), exactly the failure mode #32146's guard prevents, so it was reverted (61f9e70). This PR is scoped to the proxy-tunnel UAF; its asan lane can still intermittently hit the pre-existing #32144 leak until #32146 lands. ## Related PRs - #30606 addresses the same crash signature but patches only the `.zig` reference files, which are no longer compiled; this PR fixes the shipping Rust implementation. - #31952 fixes the same UAF by calling a new `mark_close_notified()` helper from `ProxyTunnel::shutdown` (silently setting the flag at one call site, with `close_raw` exempted). This PR instead closes the gap inside `SSLWrapper::shutdown(true)` itself, so every fast-shutdown caller (`ProxyTunnel::shutdown`, `ProxyTunnel::close_raw`, `UpgradedDuplex::close`, `WebSocketProxyTunnel::shutdown`) gets the same "no callbacks after teardown" guarantee without new wrapper API or a shutdown/close_raw asymmetry. The close callback is fired rather than suppressed, so the error teardown path keeps delivering `on_close` -> `close_and_fail` exactly once (idempotent via `closed_notified`). Test here is a deterministic single-shot repro (the test proxy reassembles TLS records and flushes tickets + response + close_notify in one write) rather than an iteration loop. --------- Co-authored-by: Ciro Spaciari MacBook <ciro@anthropic.com>
|
Closing: this fix targets |
…AF through tunnel (#32635) ## What Comprehensive stress testing of the HTTP client proxy code paths (fetch and WebSocket, both `ProxyTunnel` and `WebSocketProxyTunnel`): **661 tests** across 5 new files plus a shared adversarial proxy/origin helper and a subprocess memory-probe fixture. | File | Tests | Covers | | --- | --- | --- | | `proxy-stress-helpers.ts` | n/a | Adversarial CONNECT + absolute-form proxy (http/https outer) with per-stage RST/kill/trickle/split hooks; adversarial origin with content-length / chunked / close-delimited × identity/gzip/deflate/br/zstd, truncation at arbitrary byte offset, redirect, echo. | | `proxy-stress-matrix.test.ts` | 335 | `{http,https}` proxy × `{http,https}` origin × framing × encoding × body-size × keepalive response matrix; upload matrix across string/Uint8Array/Blob/FormData/ReadableStream/async-iterator; streamed response via `getReader()`; trickled 1-byte-per-tick downstream; split CONNECT envelope; RFC 9110 §9.3.6 ignored-header handling; hop-by-hop stripping; method matrix; cross-scheme redirects. | | `proxy-stress-lifecycle.test.ts` | 93 | Proxy RSTs client at every tunnel stage (request-received / upstream-connected / connect-replied / first-client-byte / first-upstream-byte) × both origins × both keepalive; same during upload (string + stream body); proxy drops upstream at every stage; origin RST at 0/10/60/200 response bytes; close-delimited × compression; abort at every stage; abort after headers; abort churn (200× under ASAN); ASAN-only TLS-alert-during-handshake UAF repro. | | `proxy-stress-errors.test.ts` | 51 | CONNECT 400/403/407/500/502/503/504/301/302; proxy unreachable; upstream unreachable (502 via CONNECT and absolute-form); proxy auth (missing/wrong/correct, URL and header) for all 4 combos; inner-TLS verify (CA match, no-CA fail, checkServerIdentity reject); unsupported scheme (ftp/socks4/socks5/socks5h/ws); h2-capable origin through proxy stays HTTP/1.1. | | `proxy-stress-concurrent.test.ts` | 31 | 32× parallel per combo × keepalive; tunnel reuse (sequential + auth-keyed); 12 origins through one proxy; `reject_unauthorized` pool gate (lax → strict forces fresh CONNECT); 4× concurrent 4MB echo; idle pooled tunnel receiving stray data is evicted; 12 subprocess memory probes (6 modes × 2 proxy schemes, 300-1200 iterations each, RSS-growth bounded). | | `proxy-stress-adversarial.test.ts` | 151 | Stacked split-CONNECT × chunked × compression × keepalive; origin status 200-503 × all combos; origin-facing Host/header shape; `checkServerIdentity` approve/reject/GC-loop; path/query edges; `verbose:true`; `AbortSignal.timeout`; interleaved proxy/direct to same origin; CONNECT target shape; **WebSocket proxy matrix** (ws/wss × http/https proxy) with echo, 256KB binary, RST at CONNECT, 407 without auth, rapid close, wss-via-https-proxy open/close churn under GC. | Full suite runs in ~5 min under debug+ASAN. ## Bugs found and fixed Two pre-existing bugs surfaced by the suite (both reproduce on main without any test-helper changes). Fixed here in `src/http/lib.rs`. ### 1. Streamed request body through a CONNECT tunnel never sent `fetch()` to an `https://` origin through any proxy with a `ReadableStream` / async-iterator body hangs forever: CONNECT succeeds, inner TLS handshake completes, the `Transfer-Encoding: chunked` request head is written, then nothing. String/Uint8Array/Blob/FormData bodies are fine; `http://` origins (absolute-form, no tunnel) are fine. Cause (two layers): - The `RequestStage::ProxyHeaders` arm of `on_writable` computes `has_sent_body = self.request_body().is_empty()`. For `HTTPRequestBody::Stream` the bytes buffer is always empty here, so the request jumps straight to `RequestStage::Done` and the `is_streaming_request_body` signal path a few lines below never runs. The non-proxy `send_initial_request_payload` already gates this on `matches!(original_request_body, HTTPRequestBody::Bytes(_))`. - Even once `ProxyBody` is reached, its `Stream` case calls `flush_stream` → `write_to_stream_using_buffer` → `write_to_socket(socket, …)`, which writes plaintext chunked bytes to the outer proxy socket instead of through the inner TLS session. The `Bytes` case right next to it correctly routes through `ProxyTunnel::write`. Fix: mirror the non-proxy path's `Bytes`-only `has_sent_body` check (and matching `debug_assert`) in the `ProxyHeaders` arm; in `write_to_stream_using_buffer`, route through `ProxyTunnel::write` when `self.proxy_tunnel.is_some()`, treating `WantRead`/`WantWrite` from the inner SSL as backpressure. Encrypted output reaches the outer socket via the existing `write_encrypted` callback, same as the `Bytes` path. This also covers the `HTTPThread` drain loop which calls the same `flush_stream`. ### 2. `heap-use-after-free` in `HTTPClient::on_writable` when a TLS alert is buffered with the inner handshake flight ``` READ of size 1 thread T12 (HTTP Client) #0 HTTPClient::on_writable::<true, false> src/http/lib.rs:~2856 #1 bun_http::proxy_tunnel::on_handshake src/http/ProxyTunnel.rs:450 freed by: AsyncHTTP::on_async_http_callback_raw src/http/AsyncHTTP.rs:813 HTTPClient::close_and_fail::<false> bun_http::proxy_tunnel::on_close src/http/ProxyTunnel.rs:577 SSLWrapper::handle_reading src/uws/lib.rs:1053 SSLWrapper::flush src/uws/lib.rs:669 ProxyTunnel::on_writable::<false> src/http/ProxyTunnel.rs:740 HTTPClient::on_writable::<true, false> src/http/lib.rs:~2850 bun_http::proxy_tunnel::on_handshake src/http/ProxyTunnel.rs:450 ``` If the origin closes or sends a TLS alert in the same buffer as its ServerHello flight (origin rejecting client cert, corrupted stream via misbehaving proxy, origin crash), the client's `on_handshake → on_writable → proxy.on_writable → SSLWrapper::flush → handle_reading` chain processes the alert, fires `on_close → close_and_fail`, which runs the result callback and frees the `ThreadlocalAsyncHTTP` embedding `*self`. Control returns to `on_writable`, which immediately reads `self.state.flags.is_waiting_for_cert_check` on freed memory. Same bug class already documented in `start_proxy_handshake`'s comment. Fix: `close_and_fail` → `terminate_socket` synchronously marks the outer socket closed, and the socket handle is owned by the event loop (outlives the client). Check `socket.is_closed()` immediately after `proxy.on_writable()` and return before touching `self`. Deterministic ASAN repro in `proxy-stress-lifecycle.test.ts` ("TLS alert in same buffer as inner handshake"): a CONNECT proxy that double-writes every client→upstream byte, making the origin's TLS stack abort with an alert. Looped 20× with `ASAN_OPTIONS=…:abort_on_error=1` so the HTTP-thread UAF aborts the subprocess before the main thread's clean exit wins the race. ## Verification ``` bun bd test test/js/bun/http/proxy-stress-*.test.ts # 661 pass, 0 fail (debug+ASAN, ~5min) ``` Fail-before (src/ stashed, `bun bd`): - `proxy-stress-matrix.test.ts -t "https-origin POST ReadableStream|https-origin POST async-iterator"` → 8 fail (hang/timeout) - `proxy-stress-lifecycle.test.ts -t "TLS alert in same buffer"` → 1 fail (subprocess aborts with `heap-use-after-free`) Existing `test/js/bun/http/proxy.test.ts` (48 tests) still passes. ## Related - #31959 fixed a sibling UAF on the shutdown side of the same `SSLWrapper` callback chain. - #30606 touches the same `ProxyTunnel` close path but only in the `.zig` reference files. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
What
The
SSLWrapperbacking aProxyTunnelstored a raw*HTTPClientas itshandlers.ctx. When a proxied HTTPS response completed insidehandleReading'striggerDataCallback, the result callback freed theThreadlocalAsyncHTTP(and the embeddedHTTPClient) synchronously. If the samehandleReadinginvocation then processed an SSL close/error,triggerCloseCallbackwould invokeProxyTunnel.onClosewith the now-freed pointer.The keepalive path (
detachOwner) intentionally leftwrapper.handlers.ctxstale on the assumption that no callbacks would fire while pooled — but that assumption is violated when the detach happens inside an in-flighthandleReadingcall.Fix
Make the wrapper's
ctxthe*ProxyTunnelitself — it's refcounted and held alive acrossreceive()/onWritable(), so it's always valid while callbacks can fire. Track the owningHTTPClientin a newowner: ?*HTTPClientfield that is set instart()/adopt()and cleared indetachOwner()/detachSocket(). Each callback now doesconst this = tunnel.owner orelse return;before touching the client.Found by Fuzzilli (fingerprint
f07c6abbb250e2e1).