diff --git a/src/CacheDigest.cc b/src/CacheDigest.cc index f5b1d985114..7a73e06d9c5 100644 --- a/src/CacheDigest.cc +++ b/src/CacheDigest.cc @@ -267,12 +267,55 @@ cacheDigestReport(CacheDigest * cd, const SBuf &label, StoreEntry * e) ); } +/// CacheDigest::CalcMaskSize() helper to compute digest mask size without +/// accounting for any limits or restrictions other than those imposed by +/// uint64_t type/math itself. +static uint64_t +UnsafeMaskSize(const uint64_t cap, const uint8_t bpe) +{ + Assure(bpe); + + // This limit is paranoid because no instance can store enough objects to + // exceed this maximum. + const auto maxMaskSize = std::numeric_limits::max() / 8; + + // Same as ((cap*bpe + 7)/8 > maxMaskSize) but without overflowing multiplication or sum + if (cap > (maxMaskSize*8 - 7)/bpe) + return maxMaskSize; + + return (cap*bpe + 7)/8; +} + uint32_t CacheDigest::CalcMaskSize(uint64_t cap, uint8_t bpe) { - uint64_t bitCount = (cap * bpe) + 7; - assert(bitCount < INT_MAX); // do not 31-bit overflow later - return static_cast(bitCount / 8); + // Our mask_size data member is uint32_t. That type is hard-coded in several + // places. TODO: Use a unique type name while revising related types. We + // cannot simply cap calculations at the maximum uint32_t value because we + // must also satisfy the following requirements to protect mask_size users: + // + // R1. Avoid overflows in code that does `mask_size * 8` (e.g., to compute bit positions). + // R2. Avoid overflows in code that stores `mask_size * 8` as `int`. + // R3. Avoid overflows in legacy callers that cast `mask_size` to `ssize_t`. + // R4. Avoid unreasonably large memory allocations for mask storage. + // Bug 4534 fix defined 256MB allocations as "reasonable". + // R5. Ensure that the digest capacity derived from this mask size can be + // sent to legacy installations that assert that the corresponding mask + // bit count is less than INT_MAX. + // + // Some of the current limits below are mathematically redundant (e.g., R5 + // satisfies R2), but are explicitly listed to assist with safe refactoring. + // + // For a typical 32-bit `int`, this maxMaskSize is 268'435'454 bytes. + const auto maxMaskSize = std::min({ + static_cast(std::numeric_limits::max()) / 8, // R1 + static_cast(std::numeric_limits::max()) / 8, // R2 + static_cast(std::numeric_limits::max()), // R3 + static_cast(256)*1024*1024, // R4 + static_cast(INT_MAX - 8) / 8}); // R5 + + const auto rawMaskSize = ::UnsafeMaskSize(cap, bpe); + return static_cast(std::min(rawMaskSize, maxMaskSize)); } static void diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 8660242d0fb..bfa1f12abbf 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -780,11 +780,11 @@ peerDigestSetCBlock(PeerDigest * pd, const char *buf) } /* check consistency further */ - if ((size_t)cblock.mask_size != CacheDigest::CalcMaskSize(cblock.capacity, cblock.bits_per_entry)) { - debugs(72, DBG_CRITICAL, host << " digest cblock is corrupted " << - "(mask size mismatch: " << cblock.mask_size << " ? " << - CacheDigest::CalcMaskSize(cblock.capacity, cblock.bits_per_entry) - << ")."); + const auto calculatedMaskSize = CacheDigest::CalcMaskSize(cblock.capacity, cblock.bits_per_entry); + if (size_t(cblock.mask_size) != calculatedMaskSize) { + debugs(72, DBG_CRITICAL, "ERROR: " << host << " digest cblock is corrupted or unsupported " << + "(unexpected mask size: " << cblock.mask_size << " for " << cblock.capacity << '*' << cblock.bits_per_entry << + "; expected: " << calculatedMaskSize << ")"); return 0; } @@ -799,7 +799,7 @@ peerDigestSetCBlock(PeerDigest * pd, const char *buf) * no cblock bugs below this point */ /* check size changes */ - if (pd->cd && cblock.mask_size != (ssize_t)pd->cd->mask_size) { + if (pd->cd && size_t(cblock.mask_size) != pd->cd->mask_size) { debugs(72, 2, host << " digest changed size: " << cblock.mask_size << " -> " << pd->cd->mask_size); freed_size = pd->cd->mask_size; diff --git a/src/store_digest.cc b/src/store_digest.cc index e13944f83eb..1a3a7dd8457 100644 --- a/src/store_digest.cc +++ b/src/store_digest.cc @@ -35,6 +35,7 @@ #include "util.h" #include +#include /* * local types @@ -104,10 +105,16 @@ storeDigestCalcCap() * cap = hi_cap; */ - // Bug 4534: we still have to set an upper-limit at some reasonable value though. - // this matches cacheDigestCalcMaskSize doing (cap*bpe)+7 < INT_MAX - const uint64_t absolute_max = (INT_MAX -8) / Config.digest.bits_per_entry; - if (cap > absolute_max) { + const auto bpe = Config.digest.bits_per_entry; + Assure(bpe); + + // Digest recipients recalculate mask size using received capacity and bpe + // values. Limit sent capacity value to keep legacy digest recipients safe. + const auto safeMaskSizeMax = CacheDigest::CalcMaskSize(std::numeric_limits::max(), bpe); // absolute maximum + const auto safeCapMax = uint64_t(safeMaskSizeMax) * 8 / bpe; + const auto safeCap = std::min(cap, safeCapMax); + if (cap > safeCap) { + const auto absolute_max = safeCap; // diff reducer static time_t last_loud = 0; if (last_loud < squid_curtime - 86400) { debugs(71, DBG_IMPORTANT, "WARNING: Cache Digest cannot store " << cap << " entries. Limiting to " << absolute_max); @@ -115,10 +122,9 @@ storeDigestCalcCap() } else { debugs(71, 3, "WARNING: Cache Digest cannot store " << cap << " entries. Limiting to " << absolute_max); } - cap = absolute_max; } - return cap; + return safeCap; } #endif /* USE_CACHE_DIGESTS */