zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137145989
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +954,35 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ?
this->_sm->get_server_request_header() :
+
this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0)
pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx
interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime,
field.value_get());
+ this->_send_header.field_attach(f);
+ }
Review Comment:
_send_header can't accumulate: update_write_request() destroys and recreates
it after every outbound send and every 1xx response, and a final inbound
response sets parsing_header_done, so no second header follows it.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes
stays 0.
+ int _direct_response_hdr_bytes = 0;
Review Comment:
This matches the counter it complements: every header byte counter in HttpSM
(client_request_hdr_bytes, server_request_hdr_bytes, client_response_hdr_bytes)
is an int, and header sizes are capped far below 2 GiB.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,26 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ // For logging/SDK: client_response_hdr_bytes counts only what the tunnel
wrote.
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Same as the other thread: all of HttpSM's header byte counters are int, and
header sizes are capped far below 2 GiB.
##########
tests/gold_tests/h2/replay/http2_txn_start_read_gate.replay.yaml:
##########
@@ -60,3 +60,35 @@ sessions:
encoding: plain
data: response-body
verify: {as: equal}
+
+ # A bodyless GET arrives with END_STREAM on the HEADERS frame, so there are
no
+ # DATA frames to fall back on if the read event is dropped while TXN_START is
+ # gated. Regression coverage for the direct-header-passing path.
+ - client-request:
+ frames:
+ - HEADERS:
+ headers:
+ fields:
+ - [:method, GET]
+ - [:scheme, https]
+ - [:authority, delay-txn-start.test]
+ - [:path, /read-gate-no-body]
Review Comment:
Unquoted pseudo-header names are used by many existing replays in
tests/gold_tests/h2, both the YAML loader and Proxy Verifier accept them, and
this test passes.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the
REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) &&
url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing
(RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max =
static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max =
static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c)
{ return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req
makes the exact call.
+ if (method.size() +
static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
parse_req() accepts x(foo too, since it only checks a field name's first
character. The gate now falls back to parse_req for a name whose first
character isn't a token character, and for a value with leading or trailing
whitespace, with tests for both.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0)
pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx
interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime,
field.value_get());
+ this->_send_header.field_attach(f);
Review Comment:
The previous outbound path forwarded x(foo as well, since parse_req only
checks a name's first character, and the converter still rejects control
characters and whitespace. A plugin-set field name is admin-controlled input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
