From d16fead276432a83741d5cf0ba6ffebb9662a7c4 Mon Sep 17 00:00:00 2001 From: bneradt Date: Mon, 31 Aug 2026 10:53:01 -0500 Subject: [PATCH] Rebuild well-known string indexes on cache read The well-known string table has been effectively frozen for two decades because cached objects store indexes into it, so changing the table makes every stored index denote a different string. The only remedy on offer was a cache version bump, and that was itself unsafe: the fragment offset reader is selected by comparing an object's version against the current cache version rather than against the fixed version that layout changed at, so any bump would have routed the previous release's objects through the wrong reader. This rebuilds the indexes, along with the presence bits and slot accelerators derived from them, out of the header strings the object already stores, at unmarshal time. A build can then read objects written against any table. This also corrects the reader selection to compare against that fixed version, and completes an earlier partial rebuild that missed the request method and URL scheme indexes and skipped objects served from a compressed RAM cache. Upgrading to this does not invalidate an existing cache. Stripe validation looks only at the major version, so the directory is kept, and every object already on disk is still read and served. This bumps the minor version to 24.3 only so that the reverse holds: an ATS older than 24.3 refetches objects this build writes rather than resolving their indexes against its own table. Rolling back therefore costs a refetch of whatever was written while on 24.3, and nothing that predates the upgrade. Related: #9636 Co-Authored-By: Claude Opus 5 --- .../cache-architecture/architecture.en.rst | 41 ++++ include/iocore/cache/CacheDefs.h | 24 +- include/proxy/hdrs/HTTP.h | 14 ++ include/proxy/hdrs/URL.h | 9 + src/iocore/cache/Cache.cc | 2 + src/iocore/cache/CacheRead.cc | 19 +- src/iocore/cache/CacheVC.cc | 7 +- src/proxy/hdrs/HTTP.cc | 48 ++++ src/proxy/hdrs/HdrHeap.cc | 96 ++++++++ src/proxy/hdrs/HdrToken.cc | 75 ++---- src/proxy/hdrs/URL.cc | 7 + src/proxy/hdrs/unit_tests/test_Hdrs.cc | 192 ++++++++++++++++ src/traffic_cache_tool/CacheScan.cc | 5 + .../cache/cache_wks_table_change.test.py | 71 ++++++ .../replay/cache-wks-table-change.replay.yaml | 216 ++++++++++++++++++ 15 files changed, 750 insertions(+), 76 deletions(-) create mode 100644 tests/gold_tests/cache/cache_wks_table_change.test.py create mode 100644 tests/gold_tests/cache/replay/cache-wks-table-change.replay.yaml diff --git a/doc/developer-guide/cache-architecture/architecture.en.rst b/doc/developer-guide/cache-architecture/architecture.en.rst index df7914b31a4..ded81520285 100644 --- a/doc/developer-guide/cache-architecture/architecture.en.rst +++ b/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 ---------------- diff --git a/include/iocore/cache/CacheDefs.h b/include/iocore/cache/CacheDefs.h index c5b3e304bf4..f4642f83df2 100644 --- a/include/iocore/cache/CacheDefs.h +++ b/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; diff --git a/include/proxy/hdrs/HTTP.h b/include/proxy/hdrs/HTTP.h index c23f8dc353c..10c4319f367 100644 --- a/include/proxy/hdrs/HTTP.h +++ b/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); }; diff --git a/include/proxy/hdrs/URL.h b/include/proxy/hdrs/URL.h index e8aeee0978d..22c6e341d1a 100644 --- a/include/proxy/hdrs/URL.h +++ b/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); diff --git a/src/iocore/cache/Cache.cc b/src/iocore/cache/Cache.cc index d5dcf3df76b..bbbb15e9ca0 100644 --- a/src/iocore/cache/Cache.cc +++ b/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 diff --git a/src/iocore/cache/CacheRead.cc b/src/iocore/cache/CacheRead.cc index 5c60a68d700..8445a571642 100644 --- a/src/iocore/cache/CacheRead.cc +++ b/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 diff --git a/src/iocore/cache/CacheVC.cc b/src/iocore/cache/CacheVC.cc index b7443703b14..84a6a3728a4 100644 --- a/src/iocore/cache/CacheVC.cc +++ b/src/iocore/cache/CacheVC.cc @@ -326,8 +326,11 @@ unmarshal_helper(Doc *doc, Ptr &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; } diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index 1f86b5fadfe..0ced1909601 100644 --- a/src/proxy/hdrs/HTTP.cc +++ b/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(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; diff --git a/src/proxy/hdrs/HdrHeap.cc b/src/proxy/hdrs/HdrHeap.cc index 231e789ebfc..a8ad5d94343 100644 --- a/src/proxy/hdrs/HdrHeap.cc +++ b/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 + 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((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(marshal_hdr) + HDR_HEAP_HDR_SIZE; + char *heap_end = reinterpret_cast(marshal_hdr) + marshal_hdr->m_size; + + while (obj_data < heap_end) { + HdrHeapObjImpl *obj = reinterpret_cast(obj_data); + + switch (static_cast(obj->m_type)) { + case HdrHeapObjType::URL: { + URLImpl *url = reinterpret_cast(obj); + url->m_scheme_wks_idx = test_shift_wks_idx(url->m_scheme_wks_idx); + break; + } + case HdrHeapObjType::HTTP_HEADER: { + HTTPHdrImpl *hh = reinterpret_cast(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(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(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)); diff --git a/src/proxy/hdrs/HdrToken.cc b/src/proxy/hdrs/HdrToken.cc index 24dbbd43ce1..eef28a6f7c3 100644 --- a/src/proxy/hdrs/HdrToken.cc +++ b/src/proxy/hdrs/HdrToken.cc @@ -38,20 +38,16 @@ namespace DbgCtl dbg_ctl_hdr_token{"hdr_token"}; /* - WARNING: Indexes into this array are stored on disk for cached objects. New strings must be added at the end of the array to - avoid changing the indexes of pre-existing entries, unless the cache format version number is increased. + Indexes into this array are stored inside cached objects, but they are not a format commitment: + every reader rebuilds them from the header strings the object also stores, in + HTTPHdrImpl::recompute_wks_indices(). Strings may therefore be added, removed, reordered or + edited here without invalidating anyone's cache. + + What that does require is that no ATS predating the rebuild ever read an object written against a + different table, since it would resolve the stored indexes against its own. Cache version 24.3 is + where the rebuild landed, and an older ATS rejects anything newer than its own version, so the + bump to 24.3 settles that for good. See CACHE_DB_MINOR_VERSION in iocore/cache/CacheDefs.h. */ -struct HdrTokenFrozen { - size_t count; - uint32_t fingerprint; -}; - -// When you append strings to _hdrtoken_strs, also append an entry to _hdrtoken_strs_frozen. -// This ledger ensures that WKS strings are append-only. -constexpr HdrTokenFrozen _hdrtoken_strs_frozen[] = { - {135, 0x9ea577a9u}, -}; - constexpr std::string_view _hdrtoken_strs[] = { // MIME Field names "Accept-Charset", "Accept-Encoding", "Accept-Language", "Accept-Ranges", "Accept", "Age", "Allow", @@ -129,6 +125,9 @@ constexpr std::string_view _hdrtoken_strs[] = { // RFC-9213 Targeted Cache Control "CDN-Cache-Control"}; +// MIMEField::m_wks_idx, HTTPHdrImpl's method index and URLImpl's scheme index are all int16_t. +static_assert(std::size(_hdrtoken_strs) <= INT16_MAX, "the well-known string table outgrew the type that indexes it"); + constexpr HdrTokenTypeBinding _hdrtoken_strs_type_initializers[] = { {"file", HdrTokenType::SCHEME }, {"ftp", HdrTokenType::SCHEME }, @@ -323,14 +322,6 @@ hdrtoken_ascii_toupper(unsigned char c) constexpr uint32_t HDRTOKEN_HASH_SEED = 0x811c9dc5u; // FNV-1a 32-bit offset basis -// One raw FNV-1a step. hdrtoken_hash() folds case on top of it; the frozen-ledger fingerprint -// deliberately does not. -constexpr uint32_t -hdrtoken_hash_step(uint32_t hval, unsigned char c) -{ - return (hval ^ c) * 0x01000193u; -} - // The one hash function, shared by compile-time table construction and hdrtoken_tokenize(), so the // two can never disagree. constexpr uint32_t @@ -339,51 +330,11 @@ hdrtoken_hash(std::string_view s) uint32_t hval = HDRTOKEN_HASH_SEED; for (char const c : s) { - hval = hdrtoken_hash_step(hval, hdrtoken_ascii_toupper(static_cast(c))); - } - return hval; -} - -constexpr uint32_t -hdrtoken_frozen_fingerprint(size_t count) -{ - // Hashes the raw bytes, without case folding, because case is significant for frozen entries: - // hdrtoken_method_tokenize() matches methods case-sensitively against the stored bytes. - uint32_t hval = HDRTOKEN_HASH_SEED; - - for (size_t i = 0; i < count; ++i) { - for (char const c : _hdrtoken_strs[i]) { - hval = hdrtoken_hash_step(hval, static_cast(c)); - } - hval = hdrtoken_hash_step(hval, '\0'); // fold in a terminator so entry boundaries matter + hval = (hval ^ hdrtoken_ascii_toupper(static_cast(c))) * 0x01000193u; } return hval; } -constexpr bool -hdrtoken_frozen_rows_valid() -{ - size_t prev_count = 0; - - for (auto const &f : _hdrtoken_strs_frozen) { - if (f.count <= prev_count || f.count > std::size(_hdrtoken_strs)) { - return false; - } - if (hdrtoken_frozen_fingerprint(f.count) != f.fingerprint) { - return false; - } - prev_count = f.count; - } - return true; -} - -static_assert(hdrtoken_frozen_rows_valid(), - "A frozen well-known string changed. Indexes are stored in cached objects, so entries may only be appended, " - "never inserted, reordered, removed, or edited"); -static_assert(_hdrtoken_strs_frozen[std::size(_hdrtoken_strs_frozen) - 1].count == std::size(_hdrtoken_strs), - "The well-known string table grew without being re-frozen; append a {count, fingerprint} row to " - "_hdrtoken_strs_frozen"); - constexpr size_t hdrtoken_max_literal_length() { diff --git a/src/proxy/hdrs/URL.cc b/src/proxy/hdrs/URL.cc index b0d0e5329b3..ff81579b3eb 100644 --- a/src/proxy/hdrs/URL.cc +++ b/src/proxy/hdrs/URL.cc @@ -319,6 +319,13 @@ URLImpl::unmarshal(intptr_t offset) // HDR_UNMARSHAL_STR(m_ptr_printed_string, offset); } +void +URLImpl::recompute_wks_idx() +{ + m_scheme_wks_idx = + m_ptr_scheme != nullptr ? static_cast(hdrtoken_tokenize(m_ptr_scheme, m_len_scheme, nullptr)) : int16_t{-1}; +} + void URLImpl::rehome_strings(HdrHeap *new_heap) { diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index a94f81a405d..d11ca6cfa21 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -2902,6 +2902,198 @@ TEST_CASE("HTTPInfo::unmarshal_v24_1 frag bounds checks", "[proxy][hdrtest][unma } } +// --------------------------------------------------------------------------- +// Well-known string index rebuilding. +// +// A cached object stores indexes into the well-known string table next to the strings those +// indexes stand for. A build whose table differs from the writer's would read those indexes as +// different strings, so HTTPInfo::unmarshal() rebuilds them from the strings. +// +// There is only one table in a process, so these tests stand in for a differing table by rotating +// every stored index and clearing the presence bits and slot accelerators derived from them, the +// way a build that lacked some of these strings would leave them. +// --------------------------------------------------------------------------- +namespace +{ +int16_t +rotate_wks_idx(int16_t wks_idx) +{ + return wks_idx < 0 ? wks_idx : static_cast((wks_idx + 7) % hdrtoken_num_wks); +} + +/// Rotate every well-known string index in one marshalled header heap. +void +scramble_marshalled_heap(HdrHeap *heap) +{ + char *obj_data = reinterpret_cast(heap) + sizeof(HdrHeap); + char *heap_end = reinterpret_cast(heap) + heap->m_size; + + // Objects start at the marshalled heap's data offset, which marshal() sets to the header size. + obj_data = reinterpret_cast(heap) + reinterpret_cast(heap->m_data_start); + + while (obj_data < heap_end) { + HdrHeapObjImpl *obj = reinterpret_cast(obj_data); + + REQUIRE(obj->m_length > 0); + switch (static_cast(obj->m_type)) { + case HdrHeapObjType::URL: { + URLImpl *url = reinterpret_cast(obj); + url->m_scheme_wks_idx = rotate_wks_idx(url->m_scheme_wks_idx); + break; + } + case HdrHeapObjType::HTTP_HEADER: { + HTTPHdrImpl *hh = reinterpret_cast(obj); + if (hh->m_polarity == HTTPType::REQUEST) { + hh->u.req.m_method_wks_idx = rotate_wks_idx(hh->u.req.m_method_wks_idx); + } + break; + } + case HdrHeapObjType::FIELD_BLOCK: { + MIMEFieldBlockImpl *fblock = reinterpret_cast(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 = rotate_wks_idx(field.m_wks_idx); + } + } + break; + } + case HdrHeapObjType::MIME_HEADER: { + MIMEHdrImpl *mh = reinterpret_cast(obj); + mh->m_presence_bits = MIME_PRESENCE_NONE; + mh->m_slot_accelerators[0] = 0xFFFFFFFF; + mh->m_slot_accelerators[1] = 0xFFFFFFFF; + mh->m_slot_accelerators[2] = 0xFFFFFFFF; + mh->m_slot_accelerators[3] = 0xFFFFFFFF; + break; + } + default: + break; + } + obj_data += obj->m_length; + } +} + +void +parse_request(HTTPHdr &hdr, std::string_view text) +{ + HTTPParser parser; + + http_parser_init(&parser); + hdr.create(HTTPType::REQUEST); + + char const *start = text.data(); + char const *end = text.data() + text.length(); + + REQUIRE(hdr.parse_req(&parser, &start, end, true) == ParseResult::DONE); + http_parser_clear(&parser); +} + +void +parse_response(HTTPHdr &hdr, std::string_view text) +{ + HTTPParser parser; + + http_parser_init(&parser); + hdr.create(HTTPType::RESPONSE); + + char const *start = text.data(); + char const *end = text.data() + text.length(); + + REQUIRE(hdr.parse_resp(&parser, &start, end, true) == ParseResult::DONE); + http_parser_clear(&parser); +} +} // anonymous namespace + +TEST_CASE("HTTPHdrImpl::recompute_wks_indices rebuilds from the stored strings", "[proxy][hdrtest][wks]") +{ + HTTPHdr req; + parse_request(req, "GET /a HTTP/1.1\r\nHost: example.com\r\nCache-Control: no-cache\r\nAccept: */*\r\n\r\n"sv); + req.url_get()->scheme_set(static_cast(URL_SCHEME_HTTP)); + + // Everything the header derives from the table is now wrong, as it would be had it come from a + // build whose table differed. + req.m_http->u.req.m_method_wks_idx = rotate_wks_idx(req.m_http->u.req.m_method_wks_idx); + req.m_http->u.req.m_url_impl->m_scheme_wks_idx = rotate_wks_idx(req.m_http->u.req.m_url_impl->m_scheme_wks_idx); + req.m_mime->m_presence_bits = MIME_PRESENCE_NONE; + for (MIMEFieldBlockImpl *fblock = &req.m_mime->m_first_fblock; fblock != nullptr; fblock = fblock->m_next) { + 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 = rotate_wks_idx(field.m_wks_idx); + } + } + } + CHECK(req.method_get() != "GET"sv); + CHECK(req.presence(MIME_PRESENCE_CACHE_CONTROL) == 0); + + req.m_http->recompute_wks_indices(); + + CHECK(req.method_get() == "GET"sv); + CHECK(req.method_get_wksidx() == HTTP_WKSIDX_GET); + CHECK(req.url_get()->scheme_get() == static_cast(URL_SCHEME_HTTP)); + CHECK(req.presence(MIME_PRESENCE_CACHE_CONTROL) != 0); + CHECK(req.value_get(static_cast(MIME_FIELD_CACHE_CONTROL)) == "no-cache"sv); + CHECK(req.value_get(static_cast(MIME_FIELD_HOST)) == "example.com"sv); + + req.destroy(); +} + +TEST_CASE("HTTPInfo::unmarshal rebuilds well-known string indices", "[proxy][hdrtest][wks]") +{ + HTTPHdr req; + HTTPHdr resp; + + parse_request(req, "GET /a HTTP/1.1\r\nHost: example.com\r\nAccept-Encoding: gzip\r\n\r\n"sv); + req.url_get()->scheme_set(static_cast(URL_SCHEME_HTTP)); + parse_response(resp, + "HTTP/1.1 200 OK\r\nCache-Control: max-age=300\r\nContent-Type: text/plain\r\nVary: Accept-Encoding\r\n\r\n"sv); + + HTTPInfo info; + info.create(); + info.request_set(&req); + info.response_set(&resp); + + int const len = info.marshal_length(); + // uint64_t elements so the buffer meets the alignment marshal() asserts on. + std::vector storage((len + sizeof(uint64_t) - 1) / sizeof(uint64_t), 0); + char *const buf = reinterpret_cast(storage.data()); + + REQUIRE(info.marshal(buf, len) <= len); + + HTTPCacheAlt *marshalled = reinterpret_cast(buf); + REQUIRE(marshalled->m_request_hdr.m_heap != nullptr); + REQUIRE(marshalled->m_response_hdr.m_heap != nullptr); + scramble_marshalled_heap(reinterpret_cast(buf + reinterpret_cast(marshalled->m_request_hdr.m_heap))); + scramble_marshalled_heap(reinterpret_cast(buf + reinterpret_cast(marshalled->m_response_hdr.m_heap))); + + REQUIRE(HTTPInfo::unmarshal(buf, len, nullptr) > 0); + + HTTPInfo got; + REQUIRE(got.get_handle(buf, len) > 0); + + HTTPHdr *got_req = got.request_get(); + HTTPHdr *got_resp = got.response_get(); + + CHECK(got_req->method_get() == "GET"sv); + CHECK(got_req->method_get_wksidx() == HTTP_WKSIDX_GET); + CHECK(got_req->url_get()->scheme_get() == static_cast(URL_SCHEME_HTTP)); + CHECK(got_req->value_get(static_cast(MIME_FIELD_ACCEPT_ENCODING)) == "gzip"sv); + CHECK(got_req->presence(MIME_PRESENCE_HOST) != 0); + + CHECK(got_resp->value_get(static_cast(MIME_FIELD_CACHE_CONTROL)) == "max-age=300"sv); + CHECK(got_resp->value_get(static_cast(MIME_FIELD_VARY)) == "Accept-Encoding"sv); + CHECK(got_resp->presence(MIME_PRESENCE_CACHE_CONTROL) != 0); + CHECK(got_resp->presence(MIME_PRESENCE_VARY) != 0); + // The cooked Cache-Control cache is keyed by directive name, so it survives independently, but + // it has to still agree with the rebuilt indices. + CHECK(got_resp->get_cooked_cc_mask() & MIME_COOKED_MASK_CC_MAX_AGE); + CHECK(got_resp->get_cooked_cc_max_age() == 300); + + req.destroy(); + resp.destroy(); +} + TEST_CASE("http_parse_status overflow protection", "[proxy][hdrtest]") { SECTION("valid 3-digit status codes") diff --git a/src/traffic_cache_tool/CacheScan.cc b/src/traffic_cache_tool/CacheScan.cc index c39ffbfefdf..706cfae2f3c 100644 --- a/src/traffic_cache_tool/CacheScan.cc +++ b/src/traffic_cache_tool/CacheScan.cc @@ -343,6 +343,11 @@ CacheScan::unmarshal(char *buf, int len, RefCountObj *block_ref) alt->m_response_hdr.m_mime = hh->m_fields_impl; } + // Unlike HTTPInfo::unmarshal(), this does not rebuild the well-known string indexes the object + // stores. Those indexes belong to the traffic_server that wrote the object, whose well-known + // string table need not match this tool's, so nothing here may read them; the scan reads the URL + // through its stored strings only. Anything added here that wants an index must call + // HTTPHdrImpl::recompute_wks_indices() first, which means linking libhdrs. alt->m_unmarshal_len = orig_len - len; return zret; diff --git a/tests/gold_tests/cache/cache_wks_table_change.test.py b/tests/gold_tests/cache/cache_wks_table_change.test.py new file mode 100644 index 00000000000..43bc094f943 --- /dev/null +++ b/tests/gold_tests/cache/cache_wks_table_change.test.py @@ -0,0 +1,71 @@ +''' +A cached object written against a different well-known string table is still served correctly. + +A cached object stores indexes into the well-known string table (the field index of every MIME +field, the request method's, the request URL scheme's) right next to the strings those indexes +stand for. Change the table -- add, remove or reorder a string -- and every one of those indexes +denotes a different string than it did when the object was written. HTTPInfo::unmarshal() therefore +rebuilds all of them, plus the presence bits and slot accelerators derived from them, from the +strings the object carries. + +The table is built at compile time, so a process cannot host two of them. This test stands in for +the second table with ATS_TEST_WKS_IDX_SHIFT, which makes marshalling rotate every index it writes +and clear the derived bits -- strictly worse than any real table change, since a real one leaves +the indexes self-consistent for the writer's table. + +Emitting a cached response does not on its own prove the rebuild happened, because the response is +printed from the stored strings and comes out right either way. The transactions in the replay file +instead turn on ATS *finding* particular fields in the cached header, which is what the rebuilt +indexes and presence bits are for. The sharpest of them is the revalidation: without the rebuild +ATS cannot find Etag or Last-Modified in the cached response, so it sends a plain GET where it owes +the origin a conditional one. +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = 'A cache written against a different well-known string table is still served correctly' + +# ATS_TEST_WKS_IDX_SHIFT is compiled in only when TS_HAS_TESTS is enabled. Without it the shift is +# a no-op and this test would exercise an ordinary cache hit instead of a table change. +Test.SkipUnless(Condition.HasATSFeature('TS_HAS_TESTS')) + +Test.ContinueOnFail = True + +REPLAY_FILE = 'replay/cache-wks-table-change.replay.yaml' + +server = Test.MakeVerifierServerProcess('wks-origin', REPLAY_FILE) + +ts = Test.MakeATSProcess('ts', enable_cache=True) +# Rotate every well-known string index written into a cached object. 7 is arbitrary; any non-zero +# value that is not a multiple of the table size moves every index onto some other string. +ts.Env['ATS_TEST_WKS_IDX_SHIFT'] = '7' + +ts.Disk.records_config.update( + { + 'proxy.config.http.wait_for_cache': 1, + # The RAM cache holds the object already unmarshalled, so a hit on it would never read the + # rotated indexes back. Turn it off to force the read through the marshalled bytes. + 'proxy.config.cache.ram_cache.size': 0, + }) +ts.Disk.plugin_config.AddLine('xdebug.so --enable=x-cache,via') +ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server.Variables.http_port}/') + +tr = Test.AddTestRun('Cache an object written with a rotated well-known string table, then hit it') +tr.AddVerifierClientProcess('wks-client', REPLAY_FILE, http_ports=[ts.Variables.port], other_args='--thread-limit 1') +tr.Processes.Default.StartBefore(server) +tr.Processes.Default.StartBefore(ts) +tr.StillRunningAfter = server +tr.StillRunningAfter = ts diff --git a/tests/gold_tests/cache/replay/cache-wks-table-change.replay.yaml b/tests/gold_tests/cache/replay/cache-wks-table-change.replay.yaml new file mode 100644 index 00000000000..927d606d16b --- /dev/null +++ b/tests/gold_tests/cache/replay/cache-wks-table-change.replay.yaml @@ -0,0 +1,216 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# Traffic for the well-known-string table change test. ATS runs with +# ATS_TEST_WKS_IDX_SHIFT set, so every object stored here carries well-known +# string indexes belonging to a different table than the running build's -- +# what a cache written by a differently-built ATS looks like. +# +# Reading a header back out of such an object only works if the reader rebuilds +# those indexes, along with the presence bits and slot accelerators derived from +# them, from the strings the object also stores. Emitting the cached response +# does not prove that: the response is printed from those same strings, so it +# comes out right either way. Each transaction below instead depends on ATS +# actually *finding* a specific field in the cached header, which is what goes +# through the rebuilt indexes and presence bits. +# + +meta: + version: "1.0" + +sessions: +- transactions: + + # + # 1. Cache /vary-object as the "identity" variant. The response carries Vary, + # Etag and Last-Modified, all of which later transactions must be able to + # find in the cached header. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /wks/vary-object + headers: + fields: + - [ Host, example.com ] + - [ Accept-Encoding, identity ] + - [ uuid, fill-identity ] + - [ X-Debug, "x-cache,via" ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Content-Type, "text/plain" ] + - [ Cache-Control, "max-age=300,public" ] + - [ Vary, "Accept-Encoding" ] + - [ Etag, "\"identity-variant\"" ] + - [ Last-Modified, "Tue, 01 Jan 2030 00:00:00 GMT" ] + - [ X-Variant, "identity" ] + - [ X-Wks-Custom, "not-a-well-known-string" ] + + proxy-response: + status: 200 + headers: + fields: + - [ X-Cache, { value: miss, as: equal } ] + + # + # 2. The same variant is served from cache, unchanged. This catches a reader + # that mangles the stored header outright. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /wks/vary-object + headers: + fields: + - [ Host, example.com ] + - [ Accept-Encoding, identity ] + - [ uuid, hit-identity ] + - [ X-Debug, "x-cache,via" ] + + proxy-request: + expect: absent + + server-response: + status: 502 + reason: "Bad Gateway" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + headers: + fields: + - [ X-Cache, { value: hit-fresh, as: equal } ] + - [ Content-Type, { value: "text/plain", as: equal } ] + - [ Cache-Control, { value: "max-age=300,public", as: equal } ] + - [ Vary, { value: "Accept-Encoding", as: equal } ] + - [ Etag, { value: "\"identity-variant\"", as: equal } ] + - [ Last-Modified, { value: "Tue, 01 Jan 2030 00:00:00 GMT", as: equal } ] + - [ X-Variant, { value: identity, as: equal } ] + - [ X-Wks-Custom, { value: "not-a-well-known-string", as: equal } ] + - [ Content-Length, { value: "16", as: equal } ] + + # + # 3. A different Accept-Encoding must not be served the identity variant. + # Variant selection reads the cached request and response headers, so this + # guards the alternate-selection path against a reader that returns the + # wrong strings for them. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /wks/vary-object + headers: + fields: + - [ Host, example.com ] + - [ Accept-Encoding, gzip ] + - [ uuid, fill-gzip ] + - [ X-Debug, "x-cache,via" ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Content-Type, "text/plain" ] + - [ Cache-Control, "max-age=300,public" ] + - [ Vary, "Accept-Encoding" ] + - [ Etag, "\"gzip-variant\"" ] + - [ X-Variant, "gzip" ] + + proxy-response: + status: 200 + headers: + fields: + - [ X-Variant, { value: gzip, as: equal } ] + - [ Etag, { value: "\"gzip-variant\"", as: equal } ] + + # + # 4. Cache /stale-object with max-age=0, so the next request for it has to + # revalidate. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /wks/stale-object + headers: + fields: + - [ Host, example.com ] + - [ uuid, fill-stale ] + - [ X-Debug, "x-cache,via" ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, "max-age=0,public" ] + - [ Etag, "\"stale-object\"" ] + - [ Last-Modified, "Tue, 01 Jan 2030 00:00:00 GMT" ] + + proxy-response: + status: 200 + + # + # 5. The revalidation ATS sends must carry the validators out of the cached + # response. Reading Etag and Last-Modified back is exactly the lookup that + # a stale index breaks, and without them ATS sends a plain GET instead of a + # conditional one, so the 304 below could never be produced. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /wks/stale-object + headers: + fields: + - [ Host, example.com ] + - [ uuid, revalidate-stale ] + - [ X-Debug, "x-cache,via" ] + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: "\"stale-object\"", as: equal } ] + - [ If-Modified-Since, { value: "Tue, 01 Jan 2030 00:00:00 GMT", as: equal } ] + + server-response: + status: 304 + reason: "Not Modified" + headers: + fields: + - [ Etag, "\"stale-object\"" ] + - [ Cache-Control, "max-age=0,public" ] + + proxy-response: + status: 200 + headers: + fields: + - [ Etag, { value: "\"stale-object\"", as: equal } ] + - [ Content-Length, { value: "16", as: equal } ]