zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3649548979
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -830,15 +896,36 @@ 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()) {
+ // The SM's direct-passed header (request for an H2 origin, else
response); not
+ // serialized, so there is nothing to parse.
+ 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);
+ }
+ this->_sm->clear_pending_send_header();
+ state = ParseResult::DONE;
+ } else if (this->is_outbound_connection()) {
state = this->_send_header.parse_req(&http_parser, this->_send_reader,
&bytes_used, false);
} else {
- state = this->_send_header.parse_resp(&http_parser, this->_send_reader,
&bytes_used, false);
+ state = ParseResult::CONT;
}
Review Comment:
Reopening — confirmed regression, good catch. On the inbound path I dropped
the `parse_resp()` fallback in favor of the new direct-passing setter
(`get_client_response_header()`), but `HttpSM::setup_100_continue_transfer()`
still serializes the interim response via `write_header_into_buffer()` instead
of `write_response_header_into_buffer()`. So `_client_response_header_is_ready`
is never set, `send_hdr == nullptr` in `update_write_request()`, and the
serialized 100/103 sitting in `_send_reader` falls into the new `else { state =
ParseResult::CONT; }` branch and is never emitted as a HEADERS frame.
Since 103 Early Hints and Expect:100-continue both route through
`handle_100_continue_response` -> `INTERNAL_100_RESPONSE` ->
`setup_100_continue_transfer`, this is the root cause of the `post-continue`,
`early_hints`, and `httpbin` (Expect:100-continue) autest failures. Fix:
restore the `parse_resp()` fallback for serialized interim responses (or route
`setup_100_continue_transfer` through the direct-passing setter).
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void
*data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser,
_ua.get_txn()->get_remote_reader(), &bytes_used,
-
_ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
-
t_state.http_config_param->http_request_line_max_size,
-
t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer
stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser,
_ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos,
t_state.http_config_param->strict_uri_parsing,
+
t_state.http_config_param->http_request_line_max_size,
+
t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Reopening to close the loop on validation parity. The fast path does
re-apply the checks that matter most: `strict_uri_parsing` via
`url_is_uri_compliant()` on path/query/fragment, host-character verification
via `url_parse_internet(..., verify_host_characters=true)`, and a failed 2->1.1
conversion falls back to the serialize+parse path (invalid method -> 400).
Two `parse_req` checks are *not* reproduced on the fast path: the full
`ParseRules::is_token` method check (we only reject control/WS chars via
`value_is_valid(is_control_BIT | is_ws_BIT)`, not separators) and
`validate_hdr_content_length` (duplicate/conflicting Content-Length). CR/LF are
control chars and are rejected, so this is not a request-smuggling vector, but
the method-token and Content-Length parity gaps are real. Need to confirm the
H2 layer already rejects conflicting Content-Length before this point; will
tighten the method check to `is_token` otherwise.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +627,23 @@ HttpSM::state_read_client_request_header(int event, void
*data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser,
_ua.get_txn()->get_remote_reader(), &bytes_used,
-
_ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
-
t_state.http_config_param->http_request_line_max_size,
-
t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+ ProxyTransaction *ua_txn = _ua.get_txn();
+
+ if (ua_txn->supports_direct_header_passing() &&
ua_txn->is_parsed_receive_header_ready()) {
+ // UA_FIRST_READ never fires on this path (the read buffer stays empty),
so stamp it here.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(ua_txn->parsed_receive_header());
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(
+ &http_parser, ua_txn->get_remote_reader(), &bytes_used,
_ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
t_state.http_config_param->http_hdr_field_max_size);
+ }
client_request_hdr_bytes += bytes_used;
Review Comment:
Confirmed. On the fast path `bytes_used = client_request.length_get()` is
the full header-set size, and the ParseResult::ERROR handler then uses
`bytes_used` as a request-line-length surrogate, so an oversized H2 header set
gets mapped to 414 (URI Too Long) instead of a generic 400. Fix: keep the
`REQUEST_URI_TOO_LONG` heuristic off the fast path — track request-line length
separately, or gate that branch on the serialize path.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the
target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a
REQUEST.
+ 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()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path,
where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() &&
!this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr
&& this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable;
recover anyway.
+ if (this->_sm == nullptr ||
!this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
This no longer applies after the rewrite. The `set_pre_parsed_ua_request()`
mechanism and its `ink_assert(!"pre-parsed handoff was deferred")` are gone;
handoff is now a flag on the stream (`_is_parsed_receive_header_ready`) that
the SM pulls via `parsed_receive_header()`. A deferred `signal_read_event()`
(MUTEX_TRY_LOCK miss -> reschedule via `_read_vio_event`) just leaves the flag
set for the next read — no assert, no crash. Keeping resolved.
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -201,7 +201,16 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr
&header) const
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+
+ // Match the reparse so url_print() stays byte-identical (legacy path
+ // unchanged); require full consumption, else a stray '/', '?' or '#' is
dropped.
+ const char *astart = authority.data();
Review Comment:
The fast path skips the serialize+reparse round-trip, and that reparse used
to normalize the URL a particular way. `url_print()` output — and therefore
cache keys and remap matching — has to stay byte-identical, so the 2->1.1
converter now reproduces that normalization directly: full `:authority`
consumption via `url_parse_internet` (erroring on a stray `/ ? #`), splitting
`:path` into path/query/fragment, and stripping the leading `/` that
`url_print` re-adds. And yes to your follow-up: these are 2->1.1 conversion
fixes required to make the optimization behavior-preserving, not the
optimization itself.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -283,10 +283,16 @@ Http2Stream::main_event_handler(int event, void *edata)
Http2ErrorCode
Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size)
+{
+ return this->decode_header_blocks(hpack_handle, maximum_table_size,
header_blocks, header_blocks_length);
+}
+
+Http2ErrorCode
+Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size, const uint8_t *block, uint32_t block_len)
{
Http2ErrorCode error =
- http2_decode_header_blocks(&_receive_header, (const uint8_t
*)header_blocks, header_blocks_length, nullptr, hpack_handle,
- _trailing_header_is_possible,
maximum_table_size, this->is_outbound_connection());
+ http2_decode_header_blocks(&_receive_header, block, block_len, nullptr,
hpack_handle, _trailing_header_is_possible,
+ maximum_table_size,
this->is_outbound_connection());
Review Comment:
The explicit `(block, block_len)` overload lets us decode the header block
in place from the contiguous HPACK buffer, avoiding the per-request
malloc/memcpy/free into `header_blocks` that the member-based signature forced.
That in-place decode is one of the main allocation savings in this PR.
##########
include/proxy/http/HttpSM.h:
##########
@@ -199,6 +199,20 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
void attach_client_session(ProxyTransaction *txn);
+ // Borrowed: must outlive the copy in state_read_client_request_header. Lets
HTTP/2 skip serialize+reparse.
+ void
+ set_pre_parsed_ua_request(HTTPHdr *hdr)
Review Comment:
Moot after the rewrite — `set_pre_parsed_ua_request(HTTPHdr*)` is gone. The
handoff is now via the `ProxyTransaction` interface, and the stream owns
`_receive_header` for the lifetime of the borrow, so there is no ownership
question left to solve with `shared_ptr`. Keeping resolved.
##########
include/proxy/hdrs/URL.h:
##########
@@ -215,6 +215,9 @@ ParseResult url_parse_http(HdrHeap *heap, URLImpl *url,
const char **start, cons
bool verify_host_characters);
ParseResult url_parse_http_regex(HdrHeap *heap, URLImpl *url, const char
**start, const char *end, bool copy_strings);
+// strict_uri_parsing 0 = no check; 1/2 apply the same test url_parse() does.
+bool url_is_uri_compliant(int strict_uri_parsing, std::string_view value);
Review Comment:
Agreed — moving `url_parse` to `std::string_view` (so it can reuse
`url_is_uri_compliant`) is a good cleanup, but out of scope for this PR. Good
follow-up.
--
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]
