HTTP/3: compare stream ids at full width in HQSession lookup - #13635
Draft
brbzull0 wants to merge 1 commit into
Draft
HTTP/3: compare stream ids at full width in HQSession lookup#13635brbzull0 wants to merge 1 commit into
brbzull0 wants to merge 1 commit into
Conversation
HQSession::get_transaction() matched with t->get_transaction_id() == static_cast<int>(id). get_transaction_id() returns int and QUICStreamId is a 62-bit value, so both sides narrow and two live transactions whose ids share the low 32 bits alias each other. Add get_quic_stream_id(), returning the QUICStreamId already cached in _stream_id by the constructor, and compare against that. Reaching through _info.adapter.stream() instead would reintroduce the use-after-free fixed by 13213.
Contributor
Author
|
[approve ci autest 2] |
Contributor
Author
|
[approve ci freebsd] |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
HQSession::get_transaction()(src/proxy/http3/Http3Session.cc:100) matchedtransactions with:
HQTransaction::get_transaction_id()is declaredint(it overridesProxyTransaction's, and exists mainly for compact log fields and probearguments), while
QUICStreamIdis a 62-bit value. So the left side narrows onreturn and the right side narrows in the cast, and two live transactions whose
stream ids share the low 32 bits resolve to the same entry.
This adds
HQTransaction::get_quic_stream_id()and compares against thatinstead. It returns
_stream_id, theQUICStreamIdthe constructor alreadycaches (
src/proxy/http3/Http3Transaction.cc:68):Reading the id back through
_info.adapter.stream()at call time wouldreintroduce the use-after-free that #13213 fixed, which is why the accessor uses
the cached member.
get_transaction_id()is left as-is; nothing else compares or routes by it.QUICStreamIdis already visible inHttp3Transaction.hviaiocore/net/quic/QUICStreamVCAdapter.h, so no new include is needed.I checked for other sites narrowing a stream id the same way; this was the only
one in
src/proxy/http3/andinclude/proxy/http3/.Test
No new test, and no existing test fails without this change.
The aliasing needs two concurrent transactions whose stream ids differ only
above bit 32. Client-initiated bidirectional stream ids advance by 4, and
initial_max_streams_bidi_indefaults to 100, so a stock server never issues anid above roughly 400 -- reaching the collision requires a configuration no
default deployment uses. I would rather submit this as hardening than add a test
that only passes under a hand-tuned stream limit.
Run as a no-regression check, 8/8 pass:
h3_active_timeout,h3_flow_control,h3_go_client,h3_proxy_verifier,h3_python_client,h3_sni_check,h3_stream_lifetime,quic_no_activity_timeout.test_http3is unchanged at134 assertions in 15 cases, as expected -- the lookup is not unit tested.