From cf3d8e01201902a0a5954509be32b44b26770d2c Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Wed, 16 Sep 2026 06:31:08 -0600 Subject: [PATCH] Fix the 9.2 compatibility cache key for paths that carry ";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, yet the 9.2 hasher still appended a separator. A path such as "/a;b=1" hashed as "/a;b=1;", a key 9.2 never produced, so the compatibility lookup could not find objects a 9.2 cache stored for such paths. Only add the separator when the path does not carry one, and pin the equivalence with unit tests. The fast hash path had the same separator baked in, which made it a 9.2 implementation rather than a canonical one: enabling it through url_hash_method would have made canonical keys collide with 9.2 keys and tripped the debug parity assert on the first request. Rename it to url_CryptoHash_get_fast_92 and leave it reachable only from url_CryptoHash_get_92, so the invariant holds by construction instead of by the option staying off. --- include/proxy/hdrs/URL.h | 25 ++++++ include/tscore/ink_ascii_tolower.h | 6 +- src/proxy/hdrs/URL.cc | 54 ++++++------ src/proxy/hdrs/unit_tests/test_URL.cc | 118 +++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 30 deletions(-) diff --git a/include/proxy/hdrs/URL.h b/include/proxy/hdrs/URL.h index e8aeee0978d..e4d2de03978 100644 --- a/include/proxy/hdrs/URL.h +++ b/include/proxy/hdrs/URL.h @@ -28,6 +28,7 @@ #include "proxy/hdrs/HdrHeap.h" #include "tscore/CryptoHash.h" #include "proxy/hdrs/MIME.h" +#include #include #include "tscore/ink_apidefs.h" @@ -101,6 +102,13 @@ class URLImpl : public HdrHeapObjImpl void set_type_code(unsigned int typecode); std::string_view get_params() const noexcept; void set_params(HdrHeap *heap, std::string_view value, bool copy_string); + /** Whether the path carries a deprecated ";params" segment. + + ATS 9.2 and earlier split "/path;params" into separate path and params + components. That parsing was removed, so the segment now stays inside the + path. Reproducing a 9.2 cache key has to know which form it is looking at. + */ + bool has_path_params() const noexcept; std::string_view get_query() const noexcept; void set_query(HdrHeap *heap, std::string_view value, bool copy_string); std::string_view get_fragment() const noexcept; @@ -261,6 +269,7 @@ class URL : public HdrHeapSDKHandle char *string_get_buf(char *dstbuf, int dsbuf_size, int *length = nullptr) const; void hash_get(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const; void hash_get92(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const; + bool has_path_params() const noexcept; void host_hash_get(CryptoHash *hash) const; std::string_view scheme_get() const noexcept; @@ -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 && std::memchr(m_ptr_path, ';', m_len_path) != nullptr; +} + +inline bool +URL::has_path_params() const noexcept +{ + ink_assert(valid()); + return m_url_impl->has_path_params(); +} + /*------------------------------------------------------------------------- -------------------------------------------------------------------------*/ diff --git a/include/tscore/ink_ascii_tolower.h b/include/tscore/ink_ascii_tolower.h index 8d69121796b..1a76048696c 100644 --- a/include/tscore/ink_ascii_tolower.h +++ b/include/tscore/ink_ascii_tolower.h @@ -2,9 +2,9 @@ Bulk ASCII tolower copy. - Used on the URL canonicalization fast path for cache-key digests - (src/proxy/hdrs/URL.cc::url_CryptoHash_get_fast) and any other place that - needs to fold ASCII to lowercase over a small-to-moderate buffer. + Used on header and URL canonicalization paths (HPACK, QPACK, remap) and any + other place that needs to fold ASCII to lowercase over a small-to-moderate + buffer. Semantics match a byte-at-a-time loop using ParseRules::ink_tolower(): diff --git a/src/proxy/hdrs/URL.cc b/src/proxy/hdrs/URL.cc index ae950ebfcde..14fb4c1cdf1 100644 --- a/src/proxy/hdrs/URL.cc +++ b/src/proxy/hdrs/URL.cc @@ -77,8 +77,10 @@ int URL_WKSIDX_MMST; namespace { -// Whether we should implement url_CryptoHash_get() using url_CryptoHash_get_fast(). Note that -// url_CryptoHash_get_fast() does NOT produce the same result as url_CryptoHash_get_general(). +// Whether url_CryptoHash_get_92() should use url_CryptoHash_get_fast_92(). +// The fast implementation emits the 9.2 ";" path/params separator and is +// therefore valid only for 9.2 keys; the canonical url_CryptoHash_get() +// always uses url_CryptoHash_get_general(). constexpr int url_hash_method = 0; // Buffer size for url_CryptoHash_get() and url_CryptoHash_get_92(). @@ -1709,11 +1711,18 @@ url_describe(HdrHeapObjImpl *raw, bool /* recurse ATS_UNUSED */) * * ***********************************************************************/ -// fast path for CryptoHash, HTTP, no user/password/params/query, -// no buffer overflow, no unescaping needed +// Fast path for the 9.2 CryptoHash: HTTP, no user/password/params/query, +// no buffer overflow, no unescaping needed. +// +// Emits the ";" path/params separator that 9.2 hashed between the path and +// params components, so it is valid only for url_CryptoHash_get_92(): the +// canonical url_CryptoHash_get_general() does not emit the separator, and +// using this fast path there would make canonical keys collide with 9.2 keys. +// The 9.2 caller must also ensure the path carries no literal ';' (see +// has_path_params()), otherwise the separator would be duplicated. static inline void -url_CryptoHash_get_fast(const URLImpl *url, CryptoContext &ctx, CryptoHash *hash, cache_generation_t generation) +url_CryptoHash_get_fast_92(const URLImpl *url, CryptoContext &ctx, CryptoHash *hash, cache_generation_t generation) { char buffer[BUFSIZE]; char *p; @@ -1853,19 +1862,8 @@ void url_CryptoHash_get(const URLImpl *url, CryptoHash *hash, bool ignore_query, cache_generation_t generation) { URLHashContext ctx; - if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && - ((url->m_len_user + url->m_len_password + (ignore_query ? 0 : url->m_len_query)) == 0) && - (10u + url->m_len_scheme + url->m_len_host + url->m_len_path < BUFSIZE) && - (memchr(url->m_ptr_host, '%', url->m_len_host) == nullptr) && (memchr(url->m_ptr_path, '%', url->m_len_path) == nullptr)) { - url_CryptoHash_get_fast(url, ctx, hash, generation); -#ifdef DEBUG - CryptoHash hash_general; - url_CryptoHash_get_general(url, ctx, hash_general, ignore_query, generation); - ink_assert(*hash == hash_general); -#endif - } else { - url_CryptoHash_get_general(url, ctx, *hash, ignore_query, generation); - } + // No fast path here: the only one left emits the 9.2 ";" separator. + url_CryptoHash_get_general(url, ctx, *hash, ignore_query, generation); } static inline void @@ -1899,8 +1897,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; strs[11] = "?"; // Special case for the query paramters, allowing us to ignore them if requested @@ -1912,8 +1918,8 @@ url_CryptoHash_get_general_92(const URLImpl *url, CryptoContext &ctx, CryptoHash ends[12] = nullptr; } - ends[9] = strs[9] + 1; - ends[10] = strs[10] + url->m_len_params; + ends[9] = path_has_params ? nullptr : strs[9] + 1; + ends[10] = nullptr; ends[11] = strs[11] + 1; p = buffer; @@ -1970,11 +1976,11 @@ void url_CryptoHash_get_92(const URLImpl *url, CryptoHash *hash, bool ignore_query, cache_generation_t generation) { URLHashContext ctx; - if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && + if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && !url->has_path_params() && ((url->m_len_user + url->m_len_password + url->m_len_params + (ignore_query ? 0 : url->m_len_query)) == 0) && (10u + url->m_len_scheme + url->m_len_host + url->m_len_path < BUFSIZE) && (memchr(url->m_ptr_host, '%', url->m_len_host) == nullptr) && (memchr(url->m_ptr_path, '%', url->m_len_path) == nullptr)) { - url_CryptoHash_get_fast(url, ctx, hash, generation); + url_CryptoHash_get_fast_92(url, ctx, hash, generation); #ifdef DEBUG CryptoHash hash_general; url_CryptoHash_get_general_92(url, ctx, hash_general, ignore_query, generation); diff --git a/src/proxy/hdrs/unit_tests/test_URL.cc b/src/proxy/hdrs/unit_tests/test_URL.cc index 002dd1bc34a..020b3903f28 100644 --- a/src/proxy/hdrs/unit_tests/test_URL.cc +++ b/src/proxy/hdrs/unit_tests/test_URL.cc @@ -19,6 +19,8 @@ */ #include +#include +#include #include #include @@ -766,9 +768,6 @@ std::vector get_hash_test_cases = { HAS_EQUAL_HASH, }, { - // Verifies the scheme/host SIMD-tolower path in url_CryptoHash_get_fast: - // an uppercase host with a long enough prefix to hit the 16-byte SIMD - // body should hash identically to its lowercased form. "Uppercase host: equal hashes", "http://ONE.EXAMPLE.COM/a/path?name=value", "http://one.example.com/a/path?name=value", @@ -885,6 +884,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 noexcept + { + heap->destroy(); + } +}; + +using HdrHeapPtr = std::unique_ptr; + +CryptoHash +hash92(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + + url.hash_get92(&hash); + + return hash; +} + +CryptoHash +hash_current(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + + url.hash_get(&hash); + + return hash; +} + +bool +has_params(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + return url.has_path_params(); +} +} // namespace + +TEST_CASE("UrlHashGet92 reproduces the 9.2 cache key", "[url][hash_get92]") +{ + SECTION("a path without params gains the 9.2 separator") + { + CHECK(hash92("http://foo.test/path") == hash_current("http://foo.test/path;")); + CHECK(hash92("http://foo.test/path?q=1") == hash_current("http://foo.test/path;?q=1")); + // A ';' inside the query is not a params segment: 9.2 stopped the params + // component at '?', so the path still gained a separator of its own. + CHECK(hash92("http://foo.test/a?b;c") == hash_current("http://foo.test/a;?b;c")); + } + + SECTION("a path carrying params already spells the 9.2 key") + { + CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1")); + CHECK(hash92("http://foo.test/a;b;c") == hash_current("http://foo.test/a;b;c")); + CHECK(hash92("http://foo.test/a;b=1?q=1") == hash_current("http://foo.test/a;b=1?q=1")); + } + + SECTION("the keys diverge only when the path has no params segment") + { + // This divergence is the whole reason the compatibility lookup exists. + CHECK(hash92("http://foo.test/path") != hash_current("http://foo.test/path")); + CHECK(hash92("http://foo.test/path?q=1") != hash_current("http://foo.test/path?q=1")); + // ... and when it converges there is nothing for a second lookup to find. + CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1")); + } + + SECTION("an escaped %3B is not a params segment") + { + // 9.2 split on the literal ';' only and hashed the path without unescaping, + // so "%3B" stayed in the path and the separator was still appended. The + // params check has to look at the raw path bytes, not the decoded ones. + CHECK(hash92("http://foo.test/a%3Bb=1") == hash_current("http://foo.test/a%3Bb=1;")); + CHECK(hash92("http://foo.test/a%3Bb=1?q=1") == hash_current("http://foo.test/a%3Bb=1;?q=1")); + CHECK(hash92("http://foo.test/a%3bb=1") == hash_current("http://foo.test/a%3bb=1;")); + // The escaped and literal forms were cached separately and must stay apart. + CHECK(hash92("http://foo.test/a%3Bb=1") != hash92("http://foo.test/a;b=1")); + } + + SECTION("has_path_params identifies the converging case") + { + CHECK(!has_params("http://foo.test/path")); + CHECK(!has_params("http://foo.test/a?b;c")); + CHECK(!has_params("http://foo.test/a%3Bb=1")); + CHECK(has_params("http://foo.test/a;b=1")); + CHECK(has_params("http://foo.test/a;b=1?q=1")); + } +} + // URL getters must not construct std::string_view from a nullptr pointer // (which is UB). Parts that are not present in the URL should return an // empty string_view with data() == nullptr.