Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137534868
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7196,7 +7213,15 @@ HttpSM::setup_server_send_request()
// We need a reader so bytes don't fall off the end of
// the buffer
IOBufferReader *buf_start = server_entry->write_buffer->alloc_reader();
- server_request_hdr_bytes = hdr_length =
write_header_into_buffer(&t_state.hdr_info.server_request,
server_entry->write_buffer);
+
+ if (server_txn->supports_direct_header_passing()) {
+ _server_request_header_is_ready = true;
+ // Only the write buffer is empty; retry decisions and stats need the real
size.
+ server_request_hdr_bytes = t_state.hdr_info.server_request.length_get();
+ hdr_length = 0;
Review Comment:
For an outbound HTTP/2 request without a buffered body, `hdr_length = 0`
makes the write VIO's `nbytes` zero even while its HEADERS frame is still
pending. If inactivity times out before the stream emits HEADERS,
`handle_server_setup_error()` checks `nbytes > 0` and labels it
`INACTIVE_TIMEOUT`; `is_request_retryable()` then refuses to retry a POST
because `server_request_hdr_bytes` is nonzero, although nothing reached the
origin. Track whether the outbound header was actually emitted and use that
state for timeout classification, while retaining the no-retry behavior once it
has been sent.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,70 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here, so re-apply its checks. Both run after the
REQUEST type check
+ // below, since the accessors assert 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()));
+ };
+
+ 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) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims
whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max ||
(!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) ||
ParseRules::is_ws(value.back())))) {
Review Comment:
Checking only the first character lets an HTTP/2 field such as `x(foo` take
the direct path. The old `parse_req()` path calls `mime_parser_parse()`, which
rejects `(` in a field name via `ParseRules::is_http_field_name()`
(`src/proxy/hdrs/MIME.cc:2602-2607`); the new `(foo` test covers only invalid
*first* characters. Validate every character before passing the decoded header
to HttpSM so this malformed field falls back to the former parser/rejection
path.
--
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]
