Copilot commented on code in PR #12970:
URL: https://github.com/apache/trafficserver/pull/12970#discussion_r2933993818
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -271,6 +275,7 @@ HdrTokenFieldInfo _hdrtoken_strs_field_initializers[] = {
{"Sec-WebSocket-Key", MIME_SLOTID_NONE,
MIME_PRESENCE_NONE, HdrTokenInfoFlags::NONE
},
{"Sec-WebSocket-Version", MIME_SLOTID_NONE,
MIME_PRESENCE_NONE, HdrTokenInfoFlags::NONE
},
{"CDN-Cache-Control", MIME_SLOTID_NONE,
MIME_PRESENCE_NONE, (HdrTokenInfoFlags::COMMAS |
HdrTokenInfoFlags::MULTVALS) },
+ {TS_XDEBUG_HEADER_NAME, MIME_SLOTID_NONE,
MIME_PRESENCE_NONE, HdrTokenInfoFlags::NONE
},
Review Comment:
The new WKS entry for the xdebug header sets `HdrTokenInfoFlags::NONE`,
which makes `MIMEField::supports_commas()` return false for this header (it
checks `hdrtoken_index_to_flags(..) & COMMAS` for WKS fields). That will stop
the MIME/TS API from splitting `X-Debug: a, b` into separate values, breaking
documented usage like `X-Debug: x-remap, x-cache, probe` and the parsing loop
in xdebug (which iterates `TSMimeHdrFieldValuesCount/ValueStringGet`). Set the
flags for this header to include `HdrTokenInfoFlags::COMMAS` (and likely
`MULTVALS`, consistent with similar list-valued headers).
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -128,7 +129,10 @@ const char *const _hdrtoken_strs[] = {
"zstd",
// RFC-9213 Targeted Cache Control
- "CDN-Cache-Control"};
+ "CDN-Cache-Control",
+
+ // xdebug plugin header (cmake-configurable via XDEBUG_HEADER)
+ TS_XDEBUG_HEADER_NAME};
Review Comment:
`TS_XDEBUG_HEADER_NAME` is added to `_hdrtoken_strs` (WKS) but its value is
CMake-configurable. This file explicitly warns that WKS indexes are stored on
disk for cached objects, so changing the string behind a stable index across
builds can break cache compatibility if that header ever ends up serialized.
Consider keeping the on-disk WKS list stable (e.g., always register the fixed
string "X-Debug" and let the plugin’s `--header` option handle other names), or
clearly gate/customize this in a way that forces a cache clear / cache format
bump when changed.
##########
plugins/xdebug/xdebug.cc:
##########
@@ -963,6 +964,14 @@ TSPluginInit(int argc, const char *argv[])
}
xDebugHeader.len = strlen(xDebugHeader.str);
+ // If the header name is registered as a WKS, swap in the heap pointer so
+ // TSMimeHdrFieldFind takes the O(1) WKS integer-compare path instead of
+ // the O(n) strcasecmp field-list walk.
+ if (const char *wks = hdrtoken_string_to_wks(xDebugHeader.str,
xDebugHeader.len); wks != nullptr) {
+ TSfree(const_cast<char *>(xDebugHeader.str));
+ xDebugHeader.str = wks;
+ }
Review Comment:
The comment says the default `xDebugHeader.str` is malloc’d “for consistency
for future plugin unload events”, but after the new WKS swap it can point at
the internal HdrToken heap instead. That makes the comment misleading and
increases the risk of a future unload/cleanup path incorrectly freeing a
non-heap pointer. Update the comment (or add an explicit note) to reflect that
`xDebugHeader.str` may be replaced by a non-owning WKS pointer and must not be
freed in that case.
--
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]
