Conversation
A fetch() to an https:// origin through an HTTP(S) proxy with a
ReadableStream or async-iterator request body hung forever: the
CONNECT succeeded, the inner TLS handshake completed, the request
headers (Transfer-Encoding: chunked) were written, and then nothing.
Two bugs stacked here:
- In on_writable's ProxyHeaders arm, has_sent_body was computed as
request_body().is_empty(), which is always true for the Stream
variant, so the stage jumped straight to Done and the
is_streaming_request_body signal to start pulling chunks never
fired. The non-proxy path (send_initial_request_payload) already
gated this on HTTPRequestBody::Bytes; do the same here, and relax
the debug_assert below it to allow Stream/Sendfile.
- write_to_stream_using_buffer (reached from the ProxyBody arm and
from HTTPThread's queued-write drain) wrote the chunked body to
the outer proxy socket directly, putting plaintext into the
encrypted tunnel. Route it through ProxyTunnel::write when a
tunnel is attached, same as the Bytes arm already does; treat
WantRead/WantWrite from the inner SSL as backpressure and
propagate ConnectionClosed.
Tests cover ReadableStream and async-iterator bodies over both
HTTP-proxy and HTTPS-proxy CONNECT tunnels.
|
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 (2)
Disabled knowledge base sources:
WalkthroughIn ChangesStreamed request body through CONNECT tunnel
Possibly related PRs
Suggested reviewers
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing in favor of #32635, which landed a minute earlier and carries the identical two-part fix (the On the suggested link to #19691: that issue is about |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| Err(e) if e == err!(ConnectionClosed) => return Err(e), | ||
| Err(_) => { | ||
| // WantRead/WantWrite from the inner SSL: treat as | ||
| // backpressure — queue any new data and retry on | ||
| // the next onWritable. | ||
| if !data.is_empty() { | ||
| let _ = buffer.write(data); | ||
| } | ||
| return Ok(true); | ||
| } | ||
| } | ||
| } | ||
| if !data.is_empty() { | ||
| match ProxyTunnel::write(proxy, data) { | ||
| Ok(sent) => { | ||
| self.state.request_sent_len += sent; | ||
| if sent < data.len() { | ||
| let _ = buffer.write(&data[sent..]); | ||
| return Ok(true); | ||
| } | ||
| return Ok(false); | ||
| } | ||
| Err(e) if e == err!(ConnectionClosed) => return Err(e), |
There was a problem hiding this comment.
🔴 Propagating Err(ConnectionClosed) from ProxyTunnel::write here is unsafe: on a fatal inner-TLS write error, SSLWrapper::write_data has already synchronously fired on_close → close_and_fail → fail → dispatch_result_and_reset (which resets state and may free the AsyncHTTP embedding *self) before returning the Err. The caller (write_to_stream, lib.rs:2946-2951) then touches self again — request_stream_detach() + a second close_and_fail — on a potentially-freed client whose stage guard has been reset, double-dispatching the result callback. The pre-existing ProxyTunnel::write call sites (lib.rs:3146-3148, 3215-3217) deliberately do let Ok(...) else { return; } for exactly this reason; this branch should follow the same convention and return Ok(true) on any Err, letting on_close own the failure path.
Extended reasoning...
What the bug is
The new proxy-tunnel branch in write_to_stream_using_buffer (lines 2840 and 2862) propagates Err(ConnectionClosed) from ProxyTunnel::write back up to write_to_stream, intending the caller's existing Err arm to run close_and_fail. But for a fatal inner-SSL write error, ProxyTunnel::write has already synchronously run on_close → close_and_fail → fail() → dispatch_result_and_reset() before it returns. So when control unwinds back to write_to_stream's Err arm at lib.rs:2946-2951, it touches *self (calls stream_buffer.release(), self.request_stream_detach(), self.close_and_fail(...)) on an HTTPClient that has already been failed, reset, and possibly freed.
Step-by-step proof
Setup: fetch() with a ReadableStream body to an https:// origin through any HTTP(S) proxy. Mid-upload, the origin sends a TLS alert / close_notify (e.g. it rejects an oversized body early and tears down), so the next SSL_write on the inner wrapper returns ≤0 with a non-WANT_* error (SSL_ERROR_SSL / SSL_ERROR_ZERO_RETURN / SSL_ERROR_SYSCALL).
write_to_stream(lib.rs:2944) holds&mut self: HTTPClientandstream_buffer, and callswrite_to_stream_using_buffer.- The new branch at lib.rs:2819 sees
self.proxy_tunnel.is_some()and callsProxyTunnel::write(proxy, ...)(line 2826 or 2853). ProxyTunnel::write→SSLWrapper::write_data(uws/lib.rs:761).SSL_writefails with a non-WANT_*error. At uws/lib.rs:780 it callsself.trigger_close_callback()before returningErr(WriteDataError::ConnectionClosed)at line 781.trigger_close_callback(uws/lib.rs:827-833) synchronously invokes(handlers.on_close)(ctx)=ProxyTunnel'son_close(ProxyTunnel.rs:510).- In
on_close, during request-body uploadin_progressis true butresponse_stageisProxyHeaders(notBody) and the chunked-trailers branch doesn't match, so it falls through tothis.close_and_fail(err, socket)at ProxyTunnel.rs:574/577. close_and_fail→fail()(lib.rs:3722):close_proxy_tunnel(true)(dropsself.proxy_tunnel), thestage != Done && stage != Failguard passes, setsstage = Fail, then callsdispatch_result_and_reset(true).dispatch_result_and_reset(lib.rs:1391) callsself.state.reset()— which (InternalState.rs:178) does*self = InternalState{...}, resettingstageback to its default (no longerFail) and callingoriginal_request_body.deinit()— thencallback.run(parent_async_http(), result). The codebase explicitly documents at lib.rs:3336-3342 that this callback "can free the AsyncHTTP that embeds*self. Touching self after ... would be a use-after-free."- Control unwinds:
write_datareturnsErr(ConnectionClosed)→ProxyTunnel::writereturnsErr(err!(ConnectionClosed))→ the newErr(e) if e == err!(ConnectionClosed) => return Err(e)arm at line 2840/2862 propagates it. write_to_stream'sErrarm (lib.rs:2946-2951) now runs:stream_buffer.release()— butstate.reset()already deinit'doriginal_request_body(theStreamvariant whoseThreadSafeStreamBufferthis was derived from);self.request_stream_detach()— touches*selfafter a possible free;self.close_and_fail::<IS_SSL>(err, socket)— re-entersfail(), and sincestate.reset()resetstageaway fromFail, the guard at lib.rs:3727 passes anddispatch_result_and_resetfires the result callback a second time on already-deinit'd state.
Why existing code doesn't prevent it
The stage != Done && stage != Fail guard in fail() would normally make a second close_and_fail a no-op, but dispatch_result_and_reset calls state.reset() which assigns *self = InternalState{...}, resetting stage to its default. So the guard no longer holds on the second entry. And in the case where the result callback frees the AsyncHTTP synchronously, the guard is read from freed memory.
The two pre-existing ProxyTunnel::write call sites — ProxyBody::Bytes at lib.rs:3146-3148 and ProxyHeaders at lib.rs:3215-3217 — deliberately do let Ok(...) = ProxyTunnel::write(...) else { return; } and touch nothing on Err, with the explicit comment "just wait and retry when onWritable! if closed internally will call proxy.onClose". That is the established contract: on_close owns the failure path and the caller must never touch self after an Err from ProxyTunnel::write. The new branch breaks that contract.
Impact
- Potential use-after-free of the
HTTPClient/AsyncHTTP(memory safety). - Even when not freed synchronously: double dispatch of the result callback on reset state, and
stream_buffer.release()on a deinit'd buffer.
The trigger — origin sending a TLS alert or close_notify while a streamed body chunk is being written through the inner TLS of a CONNECT tunnel — is uncommon but realistic (origin rejects request early with 4xx and tears down, idle timeout mid-upload, corrupted record stream).
Fix
Match the existing call-site convention: on any Err from ProxyTunnel::write, do not propagate. For ConnectionClosed specifically, just return Ok(true) (or buffer pending data and return Ok(true)) and let the already-fired on_close own the failure path, exactly as lib.rs:3146-3148 / 3215-3217 do. This keeps write_to_stream from ever touching self after a possible synchronous free.
1 participant
Repro
A
fetch()to anhttps://origin through any HTTP(S) proxy with aReadableStreamor async-iterator request body hangs forever. CONNECT succeeds, the inner TLS handshake completes, the request headers (Transfer-Encoding: chunked) are written, then nothing: the body never goes out and the origin waits forever. String /Uint8Array/Blob/FormDatabodies are fine;http://origins (absolute-form, no tunnel) are fine.Cause
Two bugs stacked:
In
on_writable'sProxyHeadersarm,has_sent_body = self.request_body().is_empty(). ForHTTPRequestBody::Streamthe synchronous bytes cursor is always empty, sohas_sent_headers && has_sent_bodyis true and the stage jumps straight toDone; theis_streaming_request_bodysignal to start pulling chunks from JS never fires. The non-proxysend_initial_request_payloadpath already computeshas_sent_bodyonly forHTTPRequestBody::Bytes.Even once the stage reaches
ProxyBody, that arm'sStreamcase callsflush_stream→write_to_stream_using_buffer→write_to_socket(socket, …), i.e. the outer proxy socket, writing plaintext chunked bytes into the encrypted tunnel. TheBytescase right above it correctly goes throughProxyTunnel::write.Fix
In
ProxyHeaders, computehas_sent_bodythe same way assend_initial_request_payload(only true forHTTPRequestBody::Byteswith an empty buffer). Relax thedebug_assert!(!self.request_body().is_empty())below it to also allowStream/Sendfile, matching the non-proxyBodyarm.In
write_to_stream_using_buffer, whenself.proxy_tunnel.is_some(), route both the buffered flush and new data throughProxyTunnel::writeinstead ofwrite_to_socket.WantRead/WantWritefrom the inner SSL are treated as backpressure (queue intobuffer, returnOk(true));ConnectionClosedis propagated so the caller'sclose_and_failruns. The encrypted output reaches the outer socket via the existingwrite_encryptedcallback, same as theBytespath. This also coversHTTPThread::drain_queued_writes, which reaches the same helper.Verification
Before:
After:
The full
proxy.test.ts(52 tests) and the non-proxy streamed-body suites (async-iterator-stream.test.ts,body-stream.test.ts) still pass.