Copilot commented on code in PR #13376:
URL: https://github.com/apache/trafficserver/pull/13376#discussion_r4063727153
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -728,6 +722,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 = hdrtoken_hash_step(hval, hdrtoken_ascii_toupper(uc));
+ all_valid &= (ParseRules::is_http_field_name(static_cast<char>(uc)) != 0);
+ }
+
+ *hash_out = hval;
+ *all_valid_out = all_valid;
+ return (i < maxlen) ? i : -1;
+}
Review Comment:
This new public helper dereferences `string`, `hash_out`, and
`all_valid_out` unconditionally. Adding `ink_assert(string != nullptr)`,
`ink_assert(hash_out != nullptr)`, and `ink_assert(all_valid_out != nullptr)`
(and optionally `ink_assert(maxlen >= 0)`) would make misuse fail fast and
align with the existing defensive style in `hdrtoken_tokenize()`.
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1197,17 +1197,18 @@ url_is_strictly_compliant(const char *start, const char
*end)
bool
url_is_mostly_compliant(const char *start, const char *end)
{
+ // Mode 2 accepts exactly the printable, non-space ASCII range 0x21..0x7E --
+ // equivalent to the previous isspace()/isprint() pair, but
locale-independent
+ // and call-free. This runs on every request target under the default
+ // strict_uri_parsing=2. OR-reducing an out-of-range flag over the whole
target
+ // (no early exit, no data-dependent branch) lets the compiler auto-vectorize
+ // the scan to the build's SIMD; ATS builds -O3, where clang and GCC both do.
+ unsigned char bad = 0;
for (const char *i = start; i < end; ++i) {
- if (isspace(*i)) {
- Dbg(dbg_ctl_http, "Whitespace character [0x%.2X] found in URL",
static_cast<unsigned char>(*i));
- return false;
- }
- if (!isprint(*i)) {
- Dbg(dbg_ctl_http, "Non-printable character [0x%.2X] found in URL",
static_cast<unsigned char>(*i));
- return false;
- }
+ unsigned char const c = static_cast<unsigned char>(*i);
+ bad |= static_cast<unsigned char>((c < 0x21) | (c >
0x7E));
}
Review Comment:
This change removes the previous per-byte `Dbg(dbg_ctl_http, ...)` messages
for whitespace/non-printable URL characters (they used to log the offending
byte). If those debug logs are relied on for troubleshooting malformed
request-targets, consider restoring equivalent diagnostics behind a
debug-enabled/rare-path gate (e.g., only when `bad != 0` and debug is enabled),
so the common path stays vectorizable.
##########
src/proxy/hdrs/MIME.cc:
##########
@@ -2516,14 +2537,23 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap,
MIMEHdrImpl *mh, const char
// tokenize the name //
///////////////////////
- int field_name_wks_idx = hdrtoken_tokenize(field_name.data(),
field_name.size());
-
- if (field_name_wks_idx < 0) {
- for (auto i : field_name) {
- if (!ParseRules::is_http_field_name(i)) {
- return ParseResult::ERROR;
+ int field_name_wks_idx;
+ if (name_scan_stale) {
+ // BWS trimming shortened the name after the fused scan; redo the WKS
+ // lookup and byte validation over the trimmed name.
+ field_name_wks_idx = hdrtoken_tokenize(field_name.data(),
field_name.size());
Review Comment:
`hdrtoken_tokenize` takes an `int string_len`, but `field_name.size()` is
`size_t`. In this file you already guard lengths to `<= UINT16_MAX`, so it’s
safe, but it would be better to pass an explicit
`static_cast<int>(field_name.size())` here to avoid implicit narrowing (and
potential compiler warnings) and to match the explicit cast used in the
prehashed 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]
