Fix the 9.2 compatibility cache key for paths that carry ";params"

include/proxy/hdrs/URL.h

@@ -28,6 +28,7 @@
 #include "proxy/hdrs/HdrHeap.h"
 #include "tscore/CryptoHash.h"
 #include "proxy/hdrs/MIME.h"
+#include <cstring>
 #include <string_view>
 
 #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();
+}
+
 /*-------------------------------------------------------------------------
   -------------------------------------------------------------------------*/
 

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():
 

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);

src/proxy/hdrs/unit_tests/test_URL.cc

@@ -19,6 +19,8 @@
  */
 
 #include <cstdio>
+#include <cstring>
+#include <memory>
 #include <vector>
 
 #include <catch2/catch_test_macros.hpp>
@@ -766,9 +768,6 @@ std::vector<get_hash_test_case> 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<HdrHeap, HdrHeapDeleter>;
+
+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.