Rebuild well-known string indexes when reading a cached object

doc/developer-guide/cache-architecture/architecture.en.rst

@@ -483,6 +483,47 @@ default). Objects which are in use when the write cursor is near use the same
 underlying evacuation mechanism but are handled automatically and not via the
 explicit ``pinned`` bit in :cpp:class:`Dir`.
 
+Object Versioning
+-----------------
+
+Every ``Doc`` records the cache format version that wrote it, in its ``v_major``
+and ``v_minor`` fields, taken from ``CACHE_DB_MAJOR_VERSION`` and
+``CACHE_DB_MINOR_VERSION`` in ``iocore/cache/CacheDefs.h``.
+
+Bumping the minor version does not clear the cache. Stripe validation looks only
+at the major version, and the current reader still reads every object written at
+an older minor version. What the bump buys is protection in the other direction:
+a reader rejects any object newer than itself and refetches it, rather than
+misreading a shape it does not understand.
+
+Reading an older object sometimes needs work that reading a current one does
+not. Compare against **the fixed version at which that part of the format
+changed**, never against ``CACHE_DB_VERSION``. The latter silently changes
+meaning at the next bump, and sends every object the previous release wrote down
+the wrong path. ``CACHE_DB_FRAG_OFFSET_TABLE_VERSION`` is such a fixed point.
+
+Well-Known Strings
+------------------
+
+A marshalled header stores indexes into the well-known string table
+(``proxy/hdrs/HdrToken.cc``) beside the strings those indexes stand for: the
+index of every MIME field name, of the request method, and of the request URL
+scheme, plus the presence bits and slot accelerators derived from them. Change
+the table and every stored index denotes a different string.
+
+The strings are in the object too, so the indexes are only a cache over them.
+``HTTPInfo::unmarshal()`` rebuilds all of it through
+``HTTPHdrImpl::recompute_wks_indices()`` before anything reads the header,
+unconditionally rather than on a version test, since an object written by a
+same-version build with a different table needs the same treatment as an older
+one. The ``CacheAltMagic`` check keeps this to once per marshalled buffer, on a
+read that already paid for disk I/O or a RAM-cache decompression.
+
+The table is therefore free to change without invalidating anyone's cache. The
+one requirement is that a |TS| predating the rebuild never read an object
+written against a different table; cache version 24.3 is where the rebuild
+landed, and older versions reject anything newer than themselves.
+
 Additional Notes
 ----------------
 

include/iocore/cache/CacheDefs.h

@@ -37,13 +37,35 @@ enum class CacheInitState : int {
 #define CACHE_ALT_INDEX_DEFAULT -1
 #define CACHE_ALT_REMOVED       -2
 
+// Bumping the minor version does not clear anyone's cache: stripe validation looks only at the
+// major version, and this build still reads every object written at an older minor version. What
+// it does mean is that an ATS older than this treats the objects this build writes as corrupt and
+// refetches them, so bump it whenever an object gains a shape an older ATS would misread.
+//
+// 24.2 marshalled the fragment offset table in full; see CACHE_DB_FRAG_OFFSET_TABLE_VERSION below.
+// 24.3 stopped trusting the well-known string indexes stored in an object and started rebuilding
+// them from the header strings stored alongside them, in HTTPHdrImpl::recompute_wks_indices().
+// That is what frees the well-known string table in proxy/hdrs/HdrToken.cc to change: any ATS at
+// 24.3 or newer reads objects written against any table, and anything older refuses them outright
+// rather than resolving their indexes against the wrong table.
 static const uint8_t CACHE_DB_MAJOR_VERSION = 24;
-static const uint8_t CACHE_DB_MINOR_VERSION = 2;
+static const uint8_t CACHE_DB_MINOR_VERSION = 3;
 // This is used in various comparisons because otherwise if the minor version is 0,
 // the compile fails because the condition is always true or false. Running it through
 // VersionNumber prevents that.
 extern const ts::VersionNumber CACHE_DB_VERSION;
 
+// The first version whose objects carry a complete fragment offset table. Before it, an object
+// with more than HTTPCacheAlt::N_INTEGRAL_FRAG_OFFSETS fragments marshalled only the offsets past
+// the integral ones, and the reader rebuilt the combined table; see HTTPInfo::unmarshal_v24_1().
+// Objects older than this need that reader, and objects from this version on need
+// HTTPInfo::unmarshal(). This is a fixed point in the format's history, not the current version:
+// comparing against CACHE_DB_VERSION instead would send every current object through the old
+// reader the moment the cache version is bumped for any other reason.
+static const uint8_t           CACHE_DB_FRAG_OFFSET_TABLE_MAJOR_VERSION = 24;
+static const uint8_t           CACHE_DB_FRAG_OFFSET_TABLE_MINOR_VERSION = 2;
+extern const ts::VersionNumber CACHE_DB_FRAG_OFFSET_TABLE_VERSION;
+
 static const uint8_t CACHE_DIR_MAJOR_VERSION = 18;
 static const uint8_t CACHE_DIR_MINOR_VERSION = 0;
 

include/proxy/hdrs/HTTP.h

@@ -284,6 +284,20 @@ struct HTTPHdrImpl : public HdrHeapObjImpl {
   void   move_strings(HdrStrHeap *new_heap);
   size_t strings_length();
 
+  /** Rebuild everything in this header that indexes the well-known string table.
+   *
+   * That is the request method index, the request URL's scheme index, and the field indexes,
+   * presence bits and slot accelerators of the MIME header. All of them are caches over strings
+   * that are stored in the header itself, so they can always be rebuilt, and they must be after
+   * the header is read back from a cached object: the object may have been written by a build
+   * whose well-known string table differed from this one's, in which case the stored indexes
+   * denote different strings here than they did there.
+   *
+   * Call this only once the header is fully unmarshalled. It walks the MIME field blocks, which
+   * are separate heap objects and are not usable until their own pointers have been swizzled.
+   */
+  void recompute_wks_indices();
+
   // Sanity Check Functions
   void check_strings(HeapCheck *heaps, int num_heaps);
 };

include/proxy/hdrs/URL.h

@@ -113,6 +113,15 @@ class URLImpl : public HdrHeapObjImpl
   void   rehome_strings(HdrHeap *new_heap);
   size_t strings_length();
 
+  /** Re-derive m_scheme_wks_idx from the scheme string.
+   *
+   * m_scheme_wks_idx indexes the well-known string table, and get_scheme() answers from it in
+   * preference to m_ptr_scheme, so an index left over from a table that no longer matches this
+   * build would report the wrong scheme. The scheme string itself is stored alongside it and is
+   * authoritative, so the index can always be rebuilt from it.
+   */
+  void recompute_wks_idx();
+
   // Sanity Check Functions
   void check_strings(HeapCheck *heaps, int num_heaps);
 

src/iocore/cache/Cache.cc

@@ -53,6 +53,8 @@
 extern void register_cache_stats(CacheStatsBlock *rsb, const std::string &prefix);
 
 constexpr ts::VersionNumber CACHE_DB_VERSION(CACHE_DB_MAJOR_VERSION, CACHE_DB_MINOR_VERSION);
+constexpr ts::VersionNumber CACHE_DB_FRAG_OFFSET_TABLE_VERSION(CACHE_DB_FRAG_OFFSET_TABLE_MAJOR_VERSION,
+                                                               CACHE_DB_FRAG_OFFSET_TABLE_MINOR_VERSION);
 
 // Configuration
 

src/iocore/cache/CacheRead.cc

@@ -73,17 +73,14 @@ static constexpr bool test_force_corrupt_doc = false;
 uint32_t
 CacheVC::load_http_info(CacheHTTPInfoVector *info, Doc *doc, RefCountObj *block_ptr)
 {
-  uint32_t zret = info->get_handles(doc->hdr(), doc->hlen, block_ptr);
-  if (!this->f.doc_from_ram_cache && // ram cache is always already fixed up.
-                                     // If this is an old object, the object version will be old or 0, in either case this is
-                                     // correct. Forget the 4.2 compatibility, always update older versioned objects.
-      ts::VersionNumber(doc->v_major, doc->v_minor) < CACHE_DB_VERSION) {
-    for (int i = info->xcount - 1; i >= 0; --i) {
-      info->data(i).alternate.m_alt->m_response_hdr.m_mime->recompute_accelerators_and_presence_bits();
-      info->data(i).alternate.m_alt->m_request_hdr.m_mime->recompute_accelerators_and_presence_bits();
-    }
-  }
-  return zret;
+  // The well-known string indexes, presence bits and slot accelerators these headers carry were
+  // rebuilt by HTTPInfo::unmarshal(), which every marshalled object passes through exactly once.
+  // This used to be done here instead, gated on the object being older than the running cache
+  // version and on the fragment not coming from the RAM cache. Both gates were wrong: an object
+  // written by a same-version build with a different well-known string table needs the same fixup,
+  // and with proxy.config.cache.ram_cache.compress enabled the RAM cache holds the object still
+  // marshalled, so a hit on it reaches here unfixed.
+  return info->get_handles(doc->hdr(), doc->hlen, block_ptr);
 }
 
 int

src/iocore/cache/CacheVC.cc

@@ -326,8 +326,11 @@ unmarshal_helper(Doc *doc, Ptr<IOBufferData> &buf, int &okay)
   ts::VersionNumber version(doc->v_major, doc->v_minor);
 
   // introduced by https://github.com/apache/trafficserver/pull/4874, this is used to distinguish the doc version
-  // before and after #4847
-  if (version < CACHE_DB_VERSION) {
+  // before and after #4847. Only objects written before the fragment offset table was marshalled in
+  // full need the old reader, so this compares against that fixed version rather than the current
+  // one: with CACHE_DB_VERSION here, bumping the cache version would route every object written by
+  // the previous release through a reader that rebuilds their fragment offset tables wrongly.
+  if (version < CACHE_DB_FRAG_OFFSET_TABLE_VERSION) {
     unmarshal_func = &HTTPInfo::unmarshal_v24_1;
   }
 

src/proxy/hdrs/HTTP.cc

@@ -1965,6 +1965,26 @@ HTTPHdrImpl::unmarshal(intptr_t offset)
   HDR_UNMARSHAL_PTR(m_fields_impl, MIMEHdrImpl, offset);
 }
 
+void
+HTTPHdrImpl::recompute_wks_indices()
+{
+  if (m_polarity == HTTPType::REQUEST) {
+    // http_hdr_method_get() answers from the index when it is set, so a stale index would report a
+    // different method than the one stored here. Tokenize case sensitively, exactly as the parser
+    // does, so a method that only matches a well-known string case insensitively stays untokenized.
+    u.req.m_method_wks_idx = u.req.m_ptr_method != nullptr ?
+                               static_cast<int16_t>(hdrtoken_method_tokenize(u.req.m_ptr_method, u.req.m_len_method)) :
+                               int16_t{-1};
+    if (u.req.m_url_impl != nullptr) {
+      u.req.m_url_impl->recompute_wks_idx();
+    }
+  }
+
+  if (m_fields_impl != nullptr) {
+    m_fields_impl->recompute_accelerators_and_presence_bits();
+  }
+}
+
 void
 HTTPHdrImpl::move_strings(HdrStrHeap *new_heap)
 {
@@ -2193,6 +2213,30 @@ HTTPInfo::marshal(char *buf, int len)
   return used;
 }
 
+namespace
+{
+/** Rebuild the well-known string indexes of a freshly unmarshalled alternate.
+ *
+ * The object may have been written by a build whose well-known string table differed from this
+ * one's, in which case the indexes it stores denote different strings here than they did there.
+ * Both header heaps are fully swizzled by the time this runs, which the MIME field block walk
+ * needs. Doing this in unmarshal() rather than in the cache covers every reader of a marshalled
+ * object, and the CacheAltMagic check keeps it to once per buffer.
+ */
+void
+recompute_alt_wks_indices(HTTPCacheAlt *alt)
+{
+  // m_heap stays null unless unmarshalling filled the header in, so it also says whether m_http is
+  // a pointer this process may follow rather than one left over from the writer.
+  if (alt->m_request_hdr.m_heap != nullptr) {
+    alt->m_request_hdr.m_http->recompute_wks_indices();
+  }
+  if (alt->m_response_hdr.m_heap != nullptr) {
+    alt->m_response_hdr.m_http->recompute_wks_indices();
+  }
+}
+} // anonymous namespace
+
 int
 HTTPInfo::unmarshal(char *buf, int len, RefCountObj *block_ref)
 {
@@ -2262,6 +2306,8 @@ HTTPInfo::unmarshal(char *buf, int len, RefCountObj *block_ref)
     alt->m_response_hdr.m_mime = hh->m_fields_impl;
   }
 
+  recompute_alt_wks_indices(alt);
+
   alt->m_unmarshal_len = orig_len - len;
 
   return alt->m_unmarshal_len;
@@ -2353,6 +2399,8 @@ HTTPInfo::unmarshal_v24_1(char *buf, int len, RefCountObj *block_ref)
     alt->m_response_hdr.m_mime = hh->m_fields_impl;
   }
 
+  recompute_alt_wks_indices(alt);
+
   alt->m_unmarshal_len = orig_len - len;
 
   return alt->m_unmarshal_len;

src/proxy/hdrs/HdrHeap.cc

@@ -31,6 +31,7 @@
  ****************************************************************************/
 
 #include "tscore/ink_platform.h"
+#include "tscore/ink_config.h"
 #include "tscore/Diags.h"
 #include "proxy/hdrs/HdrHeap.h"
 #include "proxy/hdrs/URL.h"
@@ -39,6 +40,8 @@
 #include "iocore/eventsystem/EThread.h"
 #include "iocore/eventsystem/Thread.h"
 
+#include <cstdlib>
+
 static constexpr size_t   MAX_LOST_STR_SPACE      = 1024;
 static constexpr uint32_t MAX_HDR_HEAP_OBJ_LENGTH = (1 << 20) - 1; ///< m_length is 20 bit
 
@@ -49,6 +52,95 @@ namespace
 {
 DbgCtl dbg_ctl_http{"http"};
 
+#if TS_HAS_TESTS
+// Test hook: how far to rotate the well-known string indexes written into a marshalled heap.
+// Zero, the default, leaves marshalling alone.
+int const test_wks_idx_shift = []() -> int {
+  char const *const value = std::getenv("ATS_TEST_WKS_IDX_SHIFT");
+
+  return value != nullptr ? atoi(value) : 0;
+}();
+
+int16_t
+test_shift_wks_idx(int16_t wks_idx)
+{
+  if (wks_idx < 0) {
+    return wks_idx;
+  }
+  // Fold the configured shift into [0, hdrtoken_num_wks) here rather than where it is read:
+  // hdrtoken_num_wks is initialized in another translation unit, so it is not dependable during
+  // this one's static initialization. Folding also keeps a negative or oversized environment value
+  // from producing an index that is not in the table.
+  int const shift = ((test_wks_idx_shift % hdrtoken_num_wks) + hdrtoken_num_wks) % hdrtoken_num_wks;
+
+  return static_cast<int16_t>((wks_idx + shift) % hdrtoken_num_wks);
+}
+
+/** Make a marshalled heap look like one written by a build with a different well-known string
+ * table: rotate every stored index, and drop the presence bits and slot accelerators that a build
+ * lacking some of this build's strings would never have set.
+ *
+ * There is no way to run two well-known string tables in one process now that the table is built at
+ * compile time, so this stands in for the case the reader has to survive. Reading such a heap back
+ * has to reproduce the header the writer had, because HTTPHdrImpl::recompute_wks_indices() rebuilds
+ * all of it from the header strings the heap also carries. See the ATS_TEST_WKS_IDX_SHIFT autest.
+ */
+void
+test_shift_marshalled_wks_indices(HdrHeap *marshal_hdr)
+{
+  if (test_wks_idx_shift == 0) {
+    return;
+  }
+
+  char *obj_data = reinterpret_cast<char *>(marshal_hdr) + HDR_HEAP_HDR_SIZE;
+  char *heap_end = reinterpret_cast<char *>(marshal_hdr) + marshal_hdr->m_size;
+
+  while (obj_data < heap_end) {
+    HdrHeapObjImpl *obj = reinterpret_cast<HdrHeapObjImpl *>(obj_data);
+
+    switch (static_cast<HdrHeapObjType>(obj->m_type)) {
+    case HdrHeapObjType::URL: {
+      URLImpl *url          = reinterpret_cast<URLImpl *>(obj);
+      url->m_scheme_wks_idx = test_shift_wks_idx(url->m_scheme_wks_idx);
+      break;
+    }
+    case HdrHeapObjType::HTTP_HEADER: {
+      HTTPHdrImpl *hh = reinterpret_cast<HTTPHdrImpl *>(obj);
+      if (hh->m_polarity == HTTPType::REQUEST) {
+        hh->u.req.m_method_wks_idx = test_shift_wks_idx(hh->u.req.m_method_wks_idx);
+      }
+      break;
+    }
+    case HdrHeapObjType::FIELD_BLOCK: {
+      MIMEFieldBlockImpl *fblock = reinterpret_cast<MIMEFieldBlockImpl *>(obj);
+      for (uint32_t i = 0; i < fblock->m_freetop; ++i) {
+        MIMEField &field = fblock->m_field_slots[i];
+        if (field.is_live()) {
+          field.m_wks_idx = test_shift_wks_idx(field.m_wks_idx);
+        }
+      }
+      break;
+    }
+    case HdrHeapObjType::MIME_HEADER: {
+      MIMEHdrImpl *mh     = reinterpret_cast<MIMEHdrImpl *>(obj);
+      mh->m_presence_bits = MIME_PRESENCE_NONE;
+      for (uint32_t &accelerator : mh->m_slot_accelerators) {
+        accelerator = 0xFFFFFFFF;
+      }
+      break;
+    }
+    default:
+      break;
+    }
+
+    if (obj->m_length <= 0) {
+      return;
+    }
+    obj_data += obj->m_length;
+  }
+}
+#endif
+
 } // end anonymous namespace
 
 /*-------------------------------------------------------------------------
@@ -817,6 +909,10 @@ HdrHeap::marshal(char *buf, int len)
     }
   }
 
+#if TS_HAS_TESTS
+  test_shift_marshalled_wks_indices(marshal_hdr);
+#endif
+
   // Add up the total bytes used
   used = ptr_heap_size + str_size + HDR_HEAP_HDR_SIZE;
   used = HdrHeapMarshalBlocks(swoc::round_up(used));