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.