Copilot commented on code in PR #13700:
URL: https://github.com/apache/trafficserver/pull/13700#discussion_r4028246599
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1711,6 +1711,11 @@ url_describe(HdrHeapObjImpl *raw, bool /* recurse
ATS_UNUSED */)
// fast path for CryptoHash, HTTP, no user/password/params/query,
// no buffer overflow, no unescaping needed
+//
+// NOTE: this emits the ";" path/params separator, which matches
+// url_CryptoHash_get_general_92() but not url_CryptoHash_get_general(). It is
+// currently unreachable because url_hash_method is 0; enabling it would make
+// canonical keys collide with 9.2 keys.
Review Comment:
The comment documents that `url_CryptoHash_get_fast()` would generate
incorrect canonical keys if `url_hash_method` were enabled. To prevent future
accidental misconfiguration, consider enforcing this invariant in code (e.g.,
only using this fast path from the 9.2 hashing path, or splitting into separate
fast implementations for canonical vs 9.2) rather than relying on the
configuration remaining unreachable.
##########
include/proxy/hdrs/URL.h:
##########
@@ -488,6 +497,22 @@ URL::hash_get92(CryptoHash *hash, bool ignore_query,
cache_generation_t generati
url_CryptoHash_get_92(m_url_impl, hash, ignore_query, generation);
}
+/*-------------------------------------------------------------------------
+ -------------------------------------------------------------------------*/
+
+inline bool
+URLImpl::has_path_params() const noexcept
+{
+ return m_ptr_path != nullptr && memchr(m_ptr_path, ';', m_len_path) !=
nullptr;
Review Comment:
`memchr` is used unqualified in a header. In C++, `<cstring>` guarantees
`std::memchr`, but availability of `::memchr` can be implementation-dependent.
Consider switching to `std::memchr` (and casting as needed) to make this header
more portable/standards-conformant.
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1899,8 +1904,16 @@ url_CryptoHash_get_general_92(const URLImpl *url,
CryptoContext &ctx, CryptoHash
ends[7] = strs[7] + 1;
ends[8] = strs[8] + url->m_len_path;
- strs[9] = ";";
- strs[10] = url->m_ptr_params;
+ // ATS 9.2 split "/path;params" into separate path and params components and
+ // hashed them as path + ";" + params. That parsing was removed, so ";params"
+ // now stays inside the path and already spells the same byte sequence.
Adding
+ // the separator again would append a ";" that 9.2 never emitted, so only add
+ // it when the path does not carry one. The params component itself is always
+ // empty now; it is left out rather than read back as an empty string.
+ bool const path_has_params = url->has_path_params();
+
+ strs[9] = path_has_params ? nullptr : ";";
+ strs[10] = nullptr;
Review Comment:
`has_path_params()` performs a `memchr` scan over the raw path each time
it’s called. If `hash_get92` is exercised frequently in production (e.g.,
during compatibility lookups), it may be worth storing a parsed/derived flag in
`URLImpl` at parse/set-path time to avoid repeated scans. If it’s truly rare,
this is fine to leave as-is.
##########
src/proxy/hdrs/unit_tests/test_URL.cc:
##########
@@ -885,6 +887,119 @@ TEST_CASE("UrlPathGet", "[url][path_get]")
}
}
+// ATS 9.2 hashed "path" ";" "params" as separate cache-key components, always
+// emitting the separator. The params component was removed from the parser, so
+// ";params" now lives inside the path and the separator is already there. That
+// makes the 9.2 key expressible through the current algorithm: it is the
+// current key of the same URL with exactly one ";" between path and query.
+namespace
+{
+/// A failing REQUIRE unwinds out of these helpers, so the heap is released by
+/// scope exit rather than by a call that the unwind would skip.
+struct HdrHeapDeleter {
+ void
+ operator()(HdrHeap *heap) const
+ {
+ heap->destroy();
+ }
+};
Review Comment:
Since this deleter is used by `std::unique_ptr`, it’s best if `operator()`
is `noexcept` to avoid potential `std::terminate` during stack
unwinding/destruction if an exception were ever thrown. Mark `operator()` as
`noexcept` (and keep it non-throwing).
--
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]
