Copilot commented on code in PR #13376:
URL: https://github.com/apache/trafficserver/pull/13376#discussion_r4126339506
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -674,21 +675,15 @@ hdrtoken_method_tokenize(const char *string, int
string_len)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
+// WKS lookup for a name whose FNV-1a hash the caller has already computed
+// (e.g. fused into the field-name scan). Matches by slot, hash, and length
like
+// hdrtoken_tokenize, but skips the interned-pointer test, so it is only valid
+// for a non-interned `string`.
int
-hdrtoken_tokenize(const char *string, int string_len, const char
**wks_string_out)
+hdrtoken_tokenize_prehashed(const char *string, int string_len, uint32_t hash,
const char **wks_string_out)
{
ink_assert(string != nullptr);
-
- if (hdrtoken_is_wks(string)) {
- int const wks_idx = hdrtoken_wks_to_index(string);
-
- if (wks_string_out) {
- *wks_string_out = string;
- }
- return wks_idx;
- }
-
- uint32_t const hash = hdrtoken_hash(std::string_view{string,
static_cast<size_t>(string_len)});
+ ink_assert(!hdrtoken_is_wks(string));
Review Comment:
`hdrtoken_tokenize_prehashed` is now part of the public header API but will
`ink_assert` if called with an interned (WKS) pointer. That makes it easy for
future callers to trip debug builds, and it differs from `hdrtoken_tokenize`
which accepts both interned and non-interned strings. Recommend either: (a)
handle the interned-pointer case in this function (mirror `hdrtoken_tokenize`
behavior) and keep the optimization for non-interned strings, or (b) move this
API out of the public header / make it explicitly internal-only (or rename to
make the precondition obvious).
##########
src/proxy/hdrs/unit_tests/test_URL.cc:
##########
@@ -172,6 +173,68 @@ TEST_CASE("ParseRulesMostlyStrictURI", "[proxy][parseuri]")
CHECK(url_is_mostly_compliant(i.uri, i.uri + strlen(i.uri)) == i.valid);
}
+namespace
+{
+// The isspace()/isprint() pair that url_is_mostly_compliant replaced, kept as
+// the reference so this compares old behavior against new instead of restating
+// the new range test. Bytes are passed as unsigned char; the original passed a
+// plain char, which is undefined for the negative values every byte >= 0x80
+// produces. In the C locale, which ATS never leaves (nothing in the tree calls
+// setlocale), this accepts exactly 0x21..0x7E.
+bool
+url_mostly_compliant_reference(const char *start, const char *end)
+{
+ for (const char *p = start; p < end; ++p) {
+ unsigned char const c = static_cast<unsigned char>(*p);
+ if (isspace(c) || !isprint(c)) {
Review Comment:
These calls use unqualified `isspace`/`isprint` after including `<cctype>`.
For portability/consistency in C++ codebases, prefer `std::isspace` /
`std::isprint` (still passing an `unsigned char`-promoted value) to avoid
relying on the global C symbols being visible.
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -708,6 +703,59 @@ hdrtoken_tokenize(const char *string, int string_len,
const char **wks_string_ou
return -1;
}
+/*-------------------------------------------------------------------------
+ -------------------------------------------------------------------------*/
+
+// Single-pass field-name scan for the MIME parser. Scans up to `maxlen` bytes
+// of `string` for the ':' delimiter while, in the same pass, accumulating the
+// FNV-1a name hash (identical to hdrtoken_hash) and tracking whether every
byte
+// before ':' is a valid HTTP field-name char. Returns the index of ':' (i.e.
+// the field-name length) or -1 if no ':' appears within `maxlen`. `*hash_out`
+// and `*all_valid_out` describe the bytes scanned before ':' (or all `maxlen`
+// bytes when ':' is absent); both are required.
+int
+hdrtoken_field_name_scan(const char *string, int maxlen, uint32_t *hash_out,
bool *all_valid_out)
+{
+ uint32_t hval = HDRTOKEN_HASH_SEED; // same FNV-1a name hash as
hdrtoken_hash
+ bool all_valid = true;
+ int i = 0;
+
+ for (; i < maxlen; ++i) {
+ unsigned char const uc = static_cast<unsigned char>(string[i]);
+ if (uc == ':') {
+ break;
+ }
+ hval = (hval ^ hdrtoken_ascii_toupper(uc)) * 0x01000193u;
+ all_valid &= (ParseRules::is_http_field_name(static_cast<char>(uc)) != 0);
Review Comment:
The validation step casts `uc` back to `char`, which reintroduces
signedness-dependent behavior for bytes >= 0x80 (on platforms where `char` is
signed). If `ParseRules::is_http_field_name` does not internally cast to
`unsigned char` before indexing/lookup, this can lead to incorrect results or
undefined behavior on non-ASCII bytes. Safer options: update
`ParseRules::is_http_field_name` (or add an overload) to accept `unsigned
char`/`uint8_t`, or ensure it performs an internal `static_cast<unsigned
char>(c)` before any table lookup, then pass `uc` through without narrowing.
##########
tools/benchmark/benchmark_HdrParse.cc:
##########
@@ -0,0 +1,927 @@
+/** @file
+
+ Micro-benchmark for HTTP header parsing (src/proxy/hdrs).
+
+ Two modes in one binary:
+
+ * A/B stats mode (default): Catch2 BENCHMARK cases produce mean/median/
+ stddev per (target x corpus) so an optimization can be measured before
+ and after and guarded against regression. Run e.g.:
+ benchmark_HdrParse "[bench]" --benchmark-samples 100
+
+ * Profiling mode (--profile <target>): a tight fixed-count loop with no
+ Catch2 harness overhead, meant to be wrapped by perf/vtune:
+ perf stat -e cycles,instructions \
+ benchmark_HdrParse --profile request --iters 5000000
+
+ Targets: request, response, mime, url, wks.
+
+ A realistic + adversarial corpus is built in. Real captured header blocks can
+ be supplied with --corpus-file FILE or --corpus-dir DIR (blocks split on a
+ blank line, classified request vs response by the first line); these are
added
+ to both modes, and each loaded request's request-target also feeds the url
+ target. In stats mode the request, response, mime, and url targets also get a
+ "corpus" benchmark that times one pass over the loaded cases alone, so its
+ result reflects only the supplied traffic. The wks benchmarks combine field
+ names from the built-in and loaded cases.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "proxy/hdrs/HTTP.h"
+#include "proxy/hdrs/MIME.h"
+#include "proxy/hdrs/URL.h"
+#include "proxy/hdrs/HdrToken.h"
+#include "proxy/hdrs/HdrHeap.h"
+
+#include <charconv>
+#include <chrono>
+#include <cstdint>
+#include <cstdio>
+#include <cstring>
+#include <filesystem>
+#include <fstream>
+#include <sstream>
+#include <string>
+#include <string_view>
+#include <utility>
+#include <vector>
+
+#define CATCH_CONFIG_ENABLE_BENCHMARKING
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/catch_session.hpp>
+#include <catch2/benchmark/catch_benchmark.hpp>
+
+// Defined in tscore; disables thread-local proxy allocators so no event-system
+// thread setup is required (mirrors the hdrs unit-test main).
+extern int cmd_disable_pfreelist;
+
+namespace
+{
+// ----------------------------------------------------------------------------
+// Corpus
+// ----------------------------------------------------------------------------
+
+struct HeaderCase {
+ std::string label;
+ std::string data; // full block including the start line, ends "\r\n\r\n"
+ bool is_response = false;
+};
+
+struct UrlCase {
+ std::string label;
+ std::string data; // a bare request target
+};
+
+struct Corpus {
+ std::vector<HeaderCase> cases;
+ std::vector<std::string> urls; // bare request targets for the url
target
+ std::vector<UrlCase> file_urls; // request targets of the file-loaded
requests
+ std::vector<std::string> wks; // field names (owned) for the wks target
+ // The same names lowercased. HTTP/2 mandates lowercase field names while the
+ // well-known-string table holds canonical mixed case, so roughly half of
real
+ // edge traffic reaches the well-known lookup folded. Without this the wks
+ // target only ever measures the canonical-case path.
+ std::vector<std::string> wks_lower;
+};
+
+// A representative modern browser request and typical responses, alongside the
+// classic fixtures used by the hdrs unit tests.
+constexpr std::string_view REQ_REALISTIC =
+ "GET /assets/app.9f2c.js HTTP/1.1\r\n"
+ "Host: www.example.com\r\n"
+ "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36
(KHTML, like Gecko) "
+ "Chrome/126.0.0.0 Safari/537.36\r\n"
+ "Accept:
text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8\r\n"
+ "Accept-Encoding: gzip, deflate, br, zstd\r\n"
+ "Accept-Language: en-US,en;q=0.9\r\n"
+ "Cookie: session=8f14e45fceea167a5a36dedd4bea2543; theme=dark;
region=us-west-2; ab_bucket=37\r\n"
+ "Referer: https://www.example.com/\r\n";
+ "Connection: keep-alive\r\n"
+ "\r\n";
+
+// A 2026 Chrome navigation request. Unlike REQ_REALISTIC (all classic WKS
+// names), this carries the modern client-hint / fetch-metadata headers
+// (sec-ch-ua*, sec-fetch-*, priority, upgrade-insecure-requests) that are NOT
+// well-known strings, so it exercises the non-WKS name paths (tokenize miss,
+// name validation, duplicate filter) the way real traffic does.
+constexpr std::string_view REQ_MODERN =
+ "GET /app/feed?tab=home HTTP/1.1\r\n"
+ "Host: www.example.com\r\n"
+ "Connection: keep-alive\r\n"
+ "sec-ch-ua: \"Chromium\";v=\"126\", \"Google Chrome\";v=\"126\",
\"Not-A.Brand\";v=\"99\"\r\n"
+ "sec-ch-ua-mobile: ?0\r\n"
+ "sec-ch-ua-platform: \"Windows\"\r\n"
+ "upgrade-insecure-requests: 1\r\n"
+ "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36
(KHTML, like Gecko) "
+ "Chrome/126.0.0.0 Safari/537.36\r\n"
+ "Accept:
text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8\r\n"
+ "Sec-Fetch-Site: same-origin\r\n"
+ "Sec-Fetch-Mode: navigate\r\n"
+ "Sec-Fetch-User: ?1\r\n"
+ "Sec-Fetch-Dest: document\r\n"
+ "Accept-Encoding: gzip, deflate, br, zstd\r\n"
+ "Accept-Language: en-US,en;q=0.9\r\n"
+ "Cookie: session=8f14e45fceea167a5a36dedd4bea2543; theme=dark;
region=us-west-2; ab_bucket=37\r\n"
+ "Priority: u=0, i\r\n"
+ "\r\n";
+
+constexpr std::string_view REQ_CLASSIC = "GET http://www.news.com:80/
HTTP/1.0\r\n"
+ "Proxy-Connection: Keep-Alive\r\n"
+ "User-Agent: Mozilla/4.04 [en] (X11;
I; Linux 2.0.33 i586)\r\n"
+ "Pragma: no-cache\r\n"
+ "Host: www.news.com\r\n"
+ "Accept: image/gif, image/x-xbitmap,
image/jpeg, image/pjpeg, image/png, */*\r\n"
+ "Accept-Language: en\r\n"
+ "Accept-Charset: iso-8859-1, *,
utf-8\r\n"
+ "\r\n";
+
+constexpr std::string_view RESP_REALISTIC = "HTTP/1.1 200 OK\r\n"
+ "Server: ATS/10.1.0\r\n"
+ "Date: Mon, 21 Oct 2013 20:13:21
GMT\r\n"
+ "Content-Type: text/html;
charset=utf-8\r\n"
+ "Content-Length: 12345\r\n"
+ "Cache-Control: max-age=31536000,
public, immutable\r\n"
+ "Vary: Accept-Encoding\r\n"
+ "Age: 42\r\n"
+ "\r\n";
+
+constexpr std::string_view RESP_304 = "HTTP/1.1 304 Not Modified\r\n"
+ "Date: Mon, 21 Oct 2013 20:13:21 GMT\r\n"
+ "Etag: \"6f2c9a1b\"\r\n"
+ "Cache-Control: max-age=31536000\r\n"
+ "\r\n";
+
+constexpr std::string_view URL_REALISTIC =
"http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75";;
+
+// Build the adversarial cases programmatically so their worst-case sizes are
+// obvious and easy to tune.
+std::string
+gen_many_fields(int n)
+{
+ std::string s = "GET /many HTTP/1.1\r\nHost: h\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "X-Custom-Header-" + std::to_string(i) + ": value-" +
std::to_string(i) + "\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+std::string
+gen_long_value(int len)
+{
+ std::string s = "GET /long HTTP/1.1\r\nHost: h\r\nX-Blob: ";
+ s.append(len, 'a');
+ s += "\r\n\r\n";
+ return s;
+}
+
+// Field names that are guaranteed not to be well-known, forcing the
+// hdrtoken_tokenize hash miss + per-char field-name validation path.
+std::string
+gen_wks_miss(int n)
+{
+ std::string s = "GET /miss HTTP/1.1\r\nHost: h\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "X-Zzq-Nonstandard-Field-" + std::to_string(i) + ": v\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+// Many duplicates of a well-known, commonly-repeated field: stresses
+// mime_hdr_field_attach duplicate chaining.
+std::string
+gen_dup_fields(int n)
+{
+ std::string s = "HTTP/1.1 200 OK\r\nDate: Mon, 21 Oct 2013 20:13:21 GMT\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "Set-Cookie: c" + std::to_string(i) + "=v" + std::to_string(i) + ";
Path=/; HttpOnly\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+std::string
+gen_long_uri(int len)
+{
+ std::string s = "GET http://www.example.com/";;
+ s.append(len, 'x');
+ s += " HTTP/1.1\r\nHost: www.example.com\r\n\r\n";
+ return s;
+}
+
+// The MIME-only target parses a field block with the start line removed.
+std::string_view
+strip_start_line(std::string_view block)
+{
+ auto pos = block.find('\n');
+ return pos == std::string_view::npos ? block : block.substr(pos + 1);
+}
+
+// Collect the field names in a block (for the wks target). Skips the start
line,
+// continuation lines, and lines without a colon.
+void
+collect_field_names(std::string_view block, std::vector<std::string> &out)
+{
+ std::string_view rest = strip_start_line(block);
+ size_t pos = 0;
+ while (pos < rest.size()) {
+ size_t eol = rest.find('\n', pos);
+ std::string_view line = rest.substr(pos, (eol == std::string_view::npos ?
rest.size() : eol) - pos);
+ pos = (eol == std::string_view::npos) ? rest.size() :
eol + 1;
+ if (line.empty() || line == "\r" || line.front() == ' ' || line.front() ==
'\t') {
+ continue; // blank terminator or folded continuation
+ }
+ size_t colon = line.find(':');
+ if (colon != std::string_view::npos && colon > 0) {
+ out.emplace_back(line.substr(0, colon));
+ }
+ }
+}
+
+// Load raw header blocks from a file. Multiple blocks may be separated by a
+// blank line. Line endings are normalized and each block re-terminated with a
+// canonical "\r\n\r\n".
+void
+load_file(const std::filesystem::path &path, std::vector<HeaderCase> &out)
+{
+ std::ifstream in(path, std::ios::binary);
+ if (!in) {
+ std::fprintf(stderr, "warning: cannot open corpus file %s\n",
path.string().c_str());
+ return;
+ }
+ std::stringstream ss;
+ ss << in.rdbuf();
+ std::string content = ss.str();
+
+ // Normalize CRLF -> LF, then split blocks on a blank line ("\n\n").
+ std::string norm;
+ norm.reserve(content.size());
+ for (char ch : content) {
+ if (ch != '\r') {
+ norm += ch;
+ }
+ }
+
+ int idx = 0;
+ size_t pos = 0;
+ while (pos < norm.size()) {
+ size_t sep = norm.find("\n\n", pos);
+ size_t end = (sep == std::string::npos) ? norm.size() : sep;
+ std::string_view chunk = std::string_view(norm).substr(pos, end - pos);
+ // Trim leading/trailing newlines.
+ while (!chunk.empty() && chunk.front() == '\n') {
+ chunk.remove_prefix(1);
+ }
+ while (!chunk.empty() && chunk.back() == '\n') {
+ chunk.remove_suffix(1);
+ }
+ if (!chunk.empty()) {
+ // Re-insert canonical CRLF line endings and terminator.
+ std::string block;
+ for (size_t i = 0; i < chunk.size(); ++i) {
+ if (chunk[i] == '\n') {
+ block += "\r\n";
+ } else {
+ block += chunk[i];
+ }
+ }
+ block += "\r\n\r\n";
+ HeaderCase c;
+ c.label = path.filename().string() + "#" + std::to_string(idx++);
+ c.is_response = block.rfind("HTTP/", 0) == 0; // status line starts with
"HTTP/"
+ c.data = std::move(block);
+ out.push_back(std::move(c));
+ }
+ if (sep == std::string::npos) {
+ break;
+ }
+ pos = sep + 2;
+ }
+}
+
+// The request-target of a request block's "METHOD SP target SP version" start
+// line, or empty if the start line doesn't have exactly that shape.
+std::string_view
+request_target(std::string_view block)
+{
+ std::string_view const line = block.substr(0, block.find("\r\n"));
+ size_t const first = line.find(' ');
+
+ if (first == std::string_view::npos) {
+ return {};
+ }
+ size_t const second = line.find(' ', first + 1);
+ if (second == std::string_view::npos || second == first + 1 || line.find('
', second + 1) != std::string_view::npos) {
+ return {};
+ }
+ return line.substr(first + 1, second - first - 1);
+}
+
+Corpus
+build_corpus(const std::vector<std::filesystem::path> &files, const
std::vector<std::filesystem::path> &dirs)
+{
+ Corpus corp;
+ auto add = [&](std::string_view label, std::string_view data, bool
is_resp) {
+ corp.cases.push_back({std::string(label), std::string(data), is_resp});
+ };
+
+ // Realistic.
+ add("req_realistic", REQ_REALISTIC, false);
+ add("req_modern", REQ_MODERN, false);
+ add("req_classic", REQ_CLASSIC, false);
+ add("resp_realistic", RESP_REALISTIC, true);
+ add("resp_304", RESP_304, true);
+
+ // Adversarial.
+ add("adv_many_fields", gen_many_fields(100), false);
+ add("adv_long_value", gen_long_value(8192), false);
+ add("adv_wks_miss", gen_wks_miss(50), false);
+ add("adv_dup_fields", gen_dup_fields(50), true);
+ add("adv_long_uri", gen_long_uri(4000), false);
+
+ // File-loaded.
+ size_t const builtin_count = corp.cases.size();
+
+ for (const auto &f : files) {
+ load_file(f, corp.cases);
+ }
+ for (const auto &d : dirs) {
+ std::error_code ec;
+ for (auto it = std::filesystem::directory_iterator(d, ec); !ec && it !=
std::filesystem::directory_iterator(); ++it) {
+ if (it->is_regular_file()) {
+ load_file(it->path(), corp.cases);
+ }
Review Comment:
`std::filesystem::directory_iterator` increment (`++it`) and
`is_regular_file()` can throw on traversal/permission errors unless you
consistently use the `error_code` overloads. Since this benchmark is designed
to be run against arbitrary user-provided corpus dirs, an exception here can
terminate the process unexpectedly. Consider switching to `it.increment(ec)`
(or the `++it` overload that takes `error_code`, depending on standard library
support) and `it->is_regular_file(ec2)` so failures are handled via `ec`
instead of exceptions (and optionally emit a warning when `ec` is set).
--
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]
![http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75"](http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75"){kind=link}