Copilot commented on code in PR #13637:
URL: https://github.com/apache/trafficserver/pull/13637#discussion_r3933401541
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -318,31 +318,45 @@ CacheVC::dead(int /* event ATS_UNUSED */, Event * /*e
ATS_UNUSED */)
return EVENT_DONE;
}
-static void
-unmarshal_helper(Doc *doc, Ptr<IOBufferData> &buf, int &okay)
+bool
+CacheVC::unmarshal_http_info(Doc *doc, Ptr<IOBufferData> &buf)
{
using UnmarshalFunc = int(char *buf, int len, RefCountObj
*block_ref);
UnmarshalFunc *unmarshal_func = &HTTPInfo::unmarshal;
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) {
+ if (version < CACHE_DB_VERSION_HTTPINFO_V24_2) {
unmarshal_func = &HTTPInfo::unmarshal_v24_1;
}
+ // Objects written by an older version can carry stale well known string
indices and
+ // presence bits. Repair them only on the MARSHALED to ALIVE transition,
since an already
+ // ALIVE block may be shared with other readers. All alts of a doc
transition together, so
+ // the first one answers for the whole header block.
+ bool const needs_wks_fixup = version < CACHE_DB_VERSION && doc->hlen > 0 &&
+ reinterpret_cast<HTTPCacheAlt
*>(doc->hdr())->m_magic == CacheAltMagic::MARSHALED;
+
char *tmp = doc->hdr();
int len = doc->hlen;
Review Comment:
`unmarshal_http_info()` walks the header block with `while (len > 0)` and
calls `HTTPInfo::unmarshal*()`, which assumes at least a full marshalled-alt
header is available. If `doc->hlen` is truncated/corrupt (non-zero but smaller
than the marshalled header), this can read past the declared header block
and/or leave a partially-unmarshalled alt in-place. Consider guarding on the
marshalled-alt size (same rounding as `HTTP_ALT_MARSHAL_SIZE`) and treating any
leftover bytes as failure.
##########
src/iocore/cache/unit_tests/test_Unmarshal_Compat.cc:
##########
@@ -0,0 +1,249 @@
+/** @file
+
+ Version compatibility tests for CacheVC::unmarshal_http_info.
+
+ @section license License
+
+ 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.
+ */
+
+#include "main.h"
+
+#include "../P_CacheDoc.h"
+#include "iocore/cache/CacheDefs.h"
+#include "proxy/hdrs/HdrHeap.h"
+#include "tscore/ink_memory.h"
+
+#include <cstring>
+#include <vector>
+
+int cache_vols = 1;
+bool reuse_existing_cache = false;
+
+namespace
+{
+int constexpr ALT_MARSHAL_SIZE =
HdrHeapMarshalBlocks{swoc::round_up(sizeof(HTTPCacheAlt))};
+int constexpr N_INTEGRAL = HTTPCacheAlt::N_INTEGRAL_FRAG_OFFSETS;
+int constexpr FRAG_COUNT = N_INTEGRAL + 2;
+
+using FragOffset = HTTPInfo::FragOffset;
+
+/** A Doc followed by a header block, versioned as an on-disk object would be.
*/
+class DocBuffer
+{
+public:
+ DocBuffer(uint8_t major, uint8_t minor, int hlen) : _storage((sizeof(Doc) +
hlen + 7) / sizeof(uint64_t) + 1, 0)
+ {
+ Doc *d = this->doc();
+
+ d->magic = DOC_MAGIC;
+ d->doc_type = CACHE_FRAG_TYPE_HTTP;
+ d->v_major = major;
+ d->v_minor = minor;
+ d->hlen = hlen;
+ d->len = sizeof(Doc) + hlen;
+ }
+
+ Doc *
+ doc()
+ {
+ return reinterpret_cast<Doc *>(_storage.data());
+ }
+
+ HTTPCacheAlt *
+ alt()
+ {
+ return reinterpret_cast<HTTPCacheAlt *>(this->doc()->hdr());
+ }
+
+private:
+ std::vector<uint64_t> _storage;
+};
+
+/** Initialize the alt header the way HTTPInfo::marshal leaves it, with no
header heaps. */
+void
+init_marshalled_alt(HTTPCacheAlt *alt, int frag_count, intptr_t
frag_table_offset)
+{
+ alt->m_magic = CacheAltMagic::MARSHALED;
+ alt->m_writeable = 0;
+ alt->m_unmarshal_len = -1;
+ alt->m_frag_offset_count = frag_count;
+
+ *reinterpret_cast<intptr_t *>(&alt->m_frag_offsets) = frag_table_offset;
+}
+
+/** The 24.2 layout: the whole fragment offset table follows the alt. */
+DocBuffer
+make_v24_2_doc(uint8_t major, uint8_t minor)
+{
+ DocBuffer buffer{major, minor, static_cast<int>(ALT_MARSHAL_SIZE +
FRAG_COUNT * sizeof(FragOffset))};
+
+ init_marshalled_alt(buffer.alt(), FRAG_COUNT, ALT_MARSHAL_SIZE);
+
+ auto *table = reinterpret_cast<FragOffset *>(buffer.doc()->hdr() +
ALT_MARSHAL_SIZE);
+
+ for (int i = 0; i < FRAG_COUNT; ++i) {
+ table[i] = i;
+ }
+ return buffer;
+}
+
+/** The 24.1 layout: the first N_INTEGRAL offsets are inline, only the rest
follow. */
+DocBuffer
+make_v24_1_doc(uint8_t major, uint8_t minor)
+{
+ int constexpr extra = FRAG_COUNT - N_INTEGRAL;
+ DocBuffer buffer{major, minor, static_cast<int>(ALT_MARSHAL_SIZE + extra *
sizeof(FragOffset))};
+
+ init_marshalled_alt(buffer.alt(), FRAG_COUNT, ALT_MARSHAL_SIZE);
+
+ for (int i = 0; i < N_INTEGRAL; ++i) {
+ buffer.alt()->m_integral_frag_offsets[i] = i;
+ }
+
+ auto *table = reinterpret_cast<FragOffset *>(buffer.doc()->hdr() +
ALT_MARSHAL_SIZE);
+
+ for (int i = 0; i < extra; ++i) {
+ table[i] = N_INTEGRAL + i;
+ }
+ return buffer;
+}
+
+void
+check_offsets_are_sequential(HTTPCacheAlt *alt)
+{
+ REQUIRE(alt->m_frag_offsets != nullptr);
+ for (int i = 0; i < FRAG_COUNT; ++i) {
+ CHECK(alt->m_frag_offsets[i] == static_cast<FragOffset>(i));
+ }
+}
+
+/** unmarshal_v24_1 copies the table onto the heap; the caller owns it. */
+void
+free_frag_offsets(HTTPCacheAlt *alt)
+{
+ if (alt->m_frag_offsets != nullptr && alt->m_frag_offsets !=
alt->m_integral_frag_offsets) {
+ ats_free(alt->m_frag_offsets);
+ alt->m_frag_offsets = nullptr;
+ }
+}
+
+} // end anonymous namespace
+
+TEST_CASE("unmarshal_http_info selects the decoder matching the object
version", "[cache][unmarshal][compat]")
+{
+ Ptr<IOBufferData> buf;
+
+ SECTION("an object at the layout boundary is read with the current decoder")
+ {
+ DocBuffer doc{make_v24_2_doc(CACHE_DB_VERSION_HTTPINFO_V24_2._major,
CACHE_DB_VERSION_HTTPINFO_V24_2._minor)};
+
+ REQUIRE(CacheVC::unmarshal_http_info(doc.doc(), buf));
+ check_offsets_are_sequential(doc.alt());
+ // Only the current decoder leaves the table in the buffer; the 24.1 one
copies it out,
+ // so this is what says which of the two ran.
+ CHECK(reinterpret_cast<char *>(doc.alt()->m_frag_offsets) ==
doc.doc()->hdr() + ALT_MARSHAL_SIZE);
+ }
+
+ SECTION("an object below the layout boundary is read with the 24.1 decoder")
+ {
+ DocBuffer doc{make_v24_1_doc(CACHE_DB_VERSION_HTTPINFO_V24_2._major,
CACHE_DB_VERSION_HTTPINFO_V24_2._minor - 1)};
+
+ // The current decoder would reject this layout outright: it expects the
whole table
+ // to follow the alt, and only the offsets past the integral ones are
there.
+ REQUIRE(CacheVC::unmarshal_http_info(doc.doc(), buf));
+ check_offsets_are_sequential(doc.alt());
+ CHECK(reinterpret_cast<char *>(doc.alt()->m_frag_offsets) !=
doc.doc()->hdr() + ALT_MARSHAL_SIZE);
+ free_frag_offsets(doc.alt());
+ }
+}
+
+TEST_CASE("unmarshal_http_info repairs stale accelerators only when it owns
the block", "[cache][unmarshal][compat]")
+{
+ HTTPInfo info;
+
+ info.create();
+ build_hdrs(info, "http://www.example.com/test.html";);
+
+ int const hlen = info.marshal_length();
+ std::vector<uint64_t> marshalled(hlen / sizeof(uint64_t) + 1, 0);
+
+ REQUIRE(info.marshal(reinterpret_cast<char *>(marshalled.data()), hlen) ==
hlen);
+
+ auto load = [&](DocBuffer &doc) { memcpy(doc.doc()->hdr(),
marshalled.data(), hlen); };
+
+ // Unmarshal an untouched copy to learn where the response MIME header lands
and what
+ // its presence bits should be. Every copy below is byte identical, so the
offset holds.
+ ptrdiff_t mime_offset = 0;
+ uint64_t correct_bits = 0;
+ {
+ DocBuffer doc{CACHE_DB_MAJOR_VERSION, CACHE_DB_MINOR_VERSION,
hlen};
+ Ptr<IOBufferData> buf;
+
+ load(doc);
+ REQUIRE(CacheVC::unmarshal_http_info(doc.doc(), buf));
+ REQUIRE(doc.alt()->m_response_hdr.valid());
+
+ mime_offset = reinterpret_cast<char *>(doc.alt()->m_response_hdr.m_mime)
- doc.doc()->hdr();
+ correct_bits = doc.alt()->m_response_hdr.m_mime->m_presence_bits;
+ REQUIRE(correct_bits != 0);
+ }
+
+ auto corrupt_presence_bits = [&](DocBuffer &doc) {
+ reinterpret_cast<MIMEHdrImpl *>(doc.doc()->hdr() +
mime_offset)->m_presence_bits = 0;
+ };
+
+ SECTION("an older object is repaired")
+ {
+ DocBuffer doc{CACHE_DB_MAJOR_VERSION, CACHE_DB_MINOR_VERSION - 1,
hlen};
Review Comment:
`CACHE_DB_MINOR_VERSION` is a `uint8_t`; `CACHE_DB_MINOR_VERSION - 1` will
underflow if the project ever bumps the cache DB major and resets minor to 0
(e.g. 25.0), making this test version "newer" than `CACHE_DB_VERSION`. Consider
computing a previous-version pair without unsigned underflow.
This issue also appears on line 236 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
