Improve cache digest mask sizing

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<uint64_t>::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<uint32_t>(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<uint64_t>(std::numeric_limits<uint32_t>::max()) / 8, // R1
+        static_cast<uint64_t>(std::numeric_limits<int>::max()) / 8, // R2
+        static_cast<uint64_t>(std::numeric_limits<ssize_t>::max()), // R3
+        static_cast<uint64_t>(256)*1024*1024, // R4
+        static_cast<uint64_t>(INT_MAX - 8) / 8}); // R5
+
+    const auto rawMaskSize = ::UnsafeMaskSize(cap, bpe);
+    return static_cast<uint32_t>(std::min(rawMaskSize, maxMaskSize));
 }
 
 static void

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;

src/store_digest.cc

@@ -35,6 +35,7 @@
 #include "util.h"
 
 #include <cmath>
+#include <limits>
 
 /*
  * local types
@@ -104,21 +105,26 @@ 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<uint64_t>::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);
             last_loud = squid_curtime;
         } 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 */