JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637665969
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char
*end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
Review Comment:
You could alternatively use `value.begin` and `value.end`, but what you did
is correct.
##########
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:
It's still not quite clear to me why the `url_print` output would have
changed.
##########
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 one needs to be checked.
##########
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:
It's out of scope for this PR, but follow-up work should update the
`url_parse` API to take a `std::string_view` for consistency (and then it can
call this helper).
##########
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:
Validation should be applied at parse-time, not on the use of the cached
value. The cached value should invariantly already be validated.
##########
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:
It's out of scope for this PR, but if performance allows, it would be nice
to take a `std::shared_ptr` here. The comment is good.
--
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]
