bneradt commented on code in PR #13637:
URL: https://github.com/apache/trafficserver/pull/13637#discussion_r3938584680
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -318,31 +319,58 @@ 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) {
+ // hlen is unsigned and the walk below is not. Narrowing a header length
this large would
+ // make the walk negative, skipping it and reporting success on a block
nothing decoded.
+ if (doc->hlen > static_cast<uint32_t>(std::numeric_limits<int>::max())) {
+ Warning("CacheVC::unmarshal_http_info: header length %u exceeds the
maximum - corrupt cache entry", doc->hlen);
+ return false;
+ }
+
+ 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 >=
sizeof(HTTPCacheAlt) &&
+ reinterpret_cast<HTTPCacheAlt
*>(doc->hdr())->m_magic == CacheAltMagic::MARSHALED;
+
char *tmp = doc->hdr();
int len = doc->hlen;
while (len > 0) {
+ // The decoders read the alt header before they check any length, so a
tail too short
+ // to hold one has to be rejected here rather than passed down.
+ if (static_cast<size_t>(len) < sizeof(HTTPCacheAlt)) {
+ Warning("CacheVC::unmarshal_http_info: header block ends mid alternate -
corrupt cache entry");
+ return false;
+ }
int r = unmarshal_func(tmp, len, buf.get());
+
if (r < 0) {
- ink_assert(!"CacheVC::handleReadDone unmarshal failed");
- okay = 0;
- break;
+ ink_assert(!"CacheVC::unmarshal_http_info: HTTPInfo unmarshal failed");
+ return false;
+ }
+ if (needs_wks_fixup) {
+ auto *alt = reinterpret_cast<HTTPCacheAlt *>(tmp);
+ for (HTTPHdr *hdr : {&alt->m_response_hdr, &alt->m_request_hdr}) {
+ if (hdr->valid()) {
+ hdr->m_mime->recompute_accelerators_and_presence_bits();
Review Comment:
`recompute_accelerators_and_presence_bits()` only rebuilds the MIME field
indexes. Two more well-known-string indexes are persisted in a marshalled
header and are not covered here:
- `HTTPHdrImpl::u.req.m_method_wks_idx`
- `URLImpl::m_scheme_wks_idx` on the request URL
Both matter because the getters prefer the index over the stored string.
`http_hdr_method_get()` returns `hdrtoken_index_to_wks(m_method_wks_idx)` when
the index is set, and `URLImpl::get_scheme()` does the same with
`m_scheme_wks_idx`, falling back to `m_ptr_scheme` only when the index is
negative. So a stale index does not merely make a lookup miss — it makes the
cached request report a different method and scheme than the one stored beside
it, which then feeds alternate selection.
I hit this in #13603 and added `HTTPHdrImpl::recompute_wks_indices()` to
cover all three in one call. Worth pulling in here even independent of the rest
of that PR, since the commit message says the fixup repairs "the well known
string indices persisted in marshalled headers" and today it repairs a subset.
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -318,31 +319,58 @@ 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) {
+ // hlen is unsigned and the walk below is not. Narrowing a header length
this large would
+ // make the walk negative, skipping it and reporting success on a block
nothing decoded.
+ if (doc->hlen > static_cast<uint32_t>(std::numeric_limits<int>::max())) {
+ Warning("CacheVC::unmarshal_http_info: header length %u exceeds the
maximum - corrupt cache entry", doc->hlen);
+ return false;
+ }
+
+ 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 >=
sizeof(HTTPCacheAlt) &&
Review Comment:
The `version < CACHE_DB_VERSION` gate is right for the problem this PR
describes, but worth being explicit that it cannot catch the case where the
writer and reader are the *same* version and their well-known string tables
differ.
That is the case #13603 is about. The indexes stored here are positions in
`_hdrtoken_strs` in `proxy/hdrs/HdrToken.cc`, and nothing ties a table revision
to a cache version, so two builds at the same `CACHE_DB_VERSION` can disagree
about what index 42 means. That is exactly why the table has been frozen since
2000 and why #9636 has sat open — changing it is what breaks the stored
indexes, and a version comparison cannot see it.
In #13603 I dropped the version test and rebuild unconditionally on the
`MARSHALED` transition. I measured the cost at ~1.3 us for a realistic 11-field
request plus 11-field response, against a path that has just done a disk read
or a decompression, so under 1% there and zero on an uncompressed RAM hit.
No change needed for this PR to be correct on its own terms. Flagging it so
the gate does not read as sufficient for all stale-index cases, and so whoever
rebases second knows this line is the one that has to change.
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -318,31 +319,58 @@ 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) {
+ // hlen is unsigned and the walk below is not. Narrowing a header length
this large would
+ // make the walk negative, skipping it and reporting success on a block
nothing decoded.
+ if (doc->hlen > static_cast<uint32_t>(std::numeric_limits<int>::max())) {
+ Warning("CacheVC::unmarshal_http_info: header length %u exceeds the
maximum - corrupt cache entry", doc->hlen);
+ return false;
+ }
+
+ 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 >=
sizeof(HTTPCacheAlt) &&
+ reinterpret_cast<HTTPCacheAlt
*>(doc->hdr())->m_magic == CacheAltMagic::MARSHALED;
+
char *tmp = doc->hdr();
int len = doc->hlen;
while (len > 0) {
+ // The decoders read the alt header before they check any length, so a
tail too short
+ // to hold one has to be rejected here rather than passed down.
+ if (static_cast<size_t>(len) < sizeof(HTTPCacheAlt)) {
+ Warning("CacheVC::unmarshal_http_info: header block ends mid alternate -
corrupt cache entry");
+ return false;
+ }
int r = unmarshal_func(tmp, len, buf.get());
+
if (r < 0) {
- ink_assert(!"CacheVC::handleReadDone unmarshal failed");
- okay = 0;
- break;
+ ink_assert(!"CacheVC::unmarshal_http_info: HTTPInfo unmarshal failed");
+ return false;
+ }
+ if (needs_wks_fixup) {
+ auto *alt = reinterpret_cast<HTTPCacheAlt *>(tmp);
+ for (HTTPHdr *hdr : {&alt->m_response_hdr, &alt->m_request_hdr}) {
+ if (hdr->valid()) {
Review Comment:
Checking `hdr->valid()` here is the right guard, and stricter than what I
wrote in #13603 — I only tested `m_heap`. Worth noting for anyone reading later
why it is needed at all: `HTTPInfo::marshal()` memcpys the whole live alt, so
`m_http` in the marshalled bytes holds a pointer from the *writing* process,
and `unmarshal` only overwrites it when the corresponding heap is non-null.
`valid()` requires all three of `m_http`, `m_mime` and `m_heap`, so it
correctly rejects that leftover.
--
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]
