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 } ]