Repository navigation
fetch: send streamed request body through CONNECT proxy tunnel #32636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Propagating
Err(ConnectionClosed)fromProxyTunnel::writehere is unsafe: on a fatal inner-TLS write error,SSLWrapper::write_datahas already synchronously firedon_close→close_and_fail→fail→dispatch_result_and_reset(which resetsstateand may free theAsyncHTTPembedding*self) before returning theErr. The caller (write_to_stream, lib.rs:2946-2951) then touchesselfagain —request_stream_detach()+ a secondclose_and_fail— on a potentially-freed client whosestageguard has been reset, double-dispatching the result callback. The pre-existingProxyTunnel::writecall sites (lib.rs:3146-3148, 3215-3217) deliberately dolet Ok(...) else { return; }for exactly this reason; this branch should follow the same convention and returnOk(true)on anyErr, lettingon_closeown the failure path.Extended reasoning...
What the bug is
The new proxy-tunnel branch in
write_to_stream_using_buffer(lines 2840 and 2862) propagatesErr(ConnectionClosed)fromProxyTunnel::writeback up towrite_to_stream, intending the caller's existingErrarm to runclose_and_fail. But for a fatal inner-SSL write error,ProxyTunnel::writehas already synchronously runon_close→close_and_fail→fail()→dispatch_result_and_reset()before it returns. So when control unwinds back towrite_to_stream'sErrarm at lib.rs:2946-2951, it touches*self(callsstream_buffer.release(),self.request_stream_detach(),self.close_and_fail(...)) on anHTTPClientthat has already been failed, reset, and possibly freed.Step-by-step proof
Setup:
fetch()with aReadableStreambody to anhttps://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 nextSSL_writeon 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.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).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."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 != Failguard infail()would normally make a secondclose_and_faila no-op, butdispatch_result_and_resetcallsstate.reset()which assigns*self = InternalState{...}, resettingstageto its default. So the guard no longer holds on the second entry. And in the case where the result callback frees theAsyncHTTPsynchronously, the guard is read from freed memory.The two pre-existing
ProxyTunnel::writecall sites —ProxyBody::Bytesat lib.rs:3146-3148 andProxyHeadersat lib.rs:3215-3217 — deliberately dolet Ok(...) = ProxyTunnel::write(...) else { return; }and touch nothing onErr, with the explicit comment "just wait and retry when onWritable! if closed internally will call proxy.onClose". That is the established contract:on_closeowns the failure path and the caller must never touchselfafter anErrfromProxyTunnel::write. The new branch breaks that contract.Impact
HTTPClient/AsyncHTTP(memory safety).stream_buffer.release()on a deinit'd buffer.The trigger — origin sending a TLS alert or
close_notifywhile 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
ErrfromProxyTunnel::write, do not propagate. ForConnectionClosedspecifically, justreturn Ok(true)(or buffer pending data andreturn Ok(true)) and let the already-firedon_closeown the failure path, exactly as lib.rs:3146-3148 / 3215-3217 do. This keepswrite_to_streamfrom ever touchingselfafter a possible synchronous free.