Copilot commented on code in PR #13768:
URL: https://github.com/apache/trafficserver/pull/13768#discussion_r4204522092
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -403,6 +403,19 @@ CacheVC::handleReadDone(int event, Event * /* e ATS_UNUSED
*/)
goto Ldone;
}
+ // Everything below trusts len and hlen, and STORE_COLLISION lets a doc
that is not ours get this far.
+ if (doc->magic == DOC_MAGIC &&
+ (doc->len < sizeof(Doc) || doc->len > io.aiocb.aio_nbytes || doc->hlen
> doc->len - sizeof(Doc))) {
+ // Callers check the key before trusting len, so a doc that is not ours
stays a collision and they keep probing.
+ // Marking it corrupt would fail a read whose own fragment may be
further down the chain.
+ if (doc->first_key == *read_key || doc->key == *read_key) {
+ Warning("Doc length %u (hlen %u) does not fit the %zu bytes read -
disk %s offset %" PRIu64, doc->len, doc->hlen,
+ static_cast<size_t>(io.aiocb.aio_nbytes),
stripe->hash_text.get(), static_cast<uint64_t>(stripe->vol_offset(&dir)));
+ doc->magic = DOC_CORRUPT;
+ }
Review Comment:
A matching malformed document is only marked `DOC_CORRUPT`, but not every
caller checks that marker. In particular, `openWriteOverwrite` checks `io.ok()`
and `first_key` only (`CacheWrite.cc:639-648`), stores this buffer in
`first_buf`, and later trusts its `len`/`hlen`. Therefore the header-update
path can still consume the out-of-bounds lengths this guard is intended to
reject. Mark the read result failed for matching malformed documents so all
existing callers stop before accepting the buffer.
--
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]
