Improve cache digest mask sizing by kinkie · Pull Request #2485 · squid-cache/squid

kinkie · 2026-08-27T06:35:53Z

This change fixes cache digest capacity and mask size calculations to
improve validation of received cache digest metadata and to prevent
generation of cache digests that may trigger problems in both
digest-generating instances and digest-receiving peers.

This change does not affect Cache Digest population algorithm and
exchange protocol. This change preserves compatibility across old and
updated peers:

Old Squids accept all digests generated by new code.
New Squids accept non-problematic digests generated by old code.
New Squids safely reject problematic digests generated by old code.

The text below details new and updated digest sizing limits.

A Squid instance may deal with two sources of Cache Digests:

A digest of the local instance caches. Since 2016 commit 831e953, its
mask size is capped at approximately INT_MAX bits. That maximum
usually corresponds to a ~256MB digest mask memory allocation. See
absolute_max calculation in old storeDigestCalcCap() code.
Digests sent to the instance by its cache_peers. These digests come
with peer-set capacity and mask sizes. The received mask size was
effectively capped at similar levels (using an assertion) when
peerDigestSetCBlock() called old CacheDigest::CalcMaskSize().

The two poorly duplicated limits were tied together by a stale C++
comment. This change removes that duplication, moving limit enforcement
from storeDigestCalcCap() into CalcMaskSize() and eliminating the
problematic assertion. If Squid receives a cache digest that violates
the new limits, that digest will be safely rejected with a level-0
"digest cblock is corrupted or unsupported" ERROR.

Also fixed integer overflow in CacheDigest::CalcMaskSize() calculations,
addressing an XXX comment correctly added in 2015 commit 5bc5e81 that
was incorrectly replaced with an incorrect assertion in 2016 commit
831e953. See new UnsafeMaskSize().

Also explicitly capped digest capacity and mask size to prevent integer
overflows where the calculated mask size is used, including uses in old
receiver code. See R1-R5 limits in updated CalcMaskSize(). Until all
Squids are upgraded, older installations will continue to receive
digests generated by upgraded Squids. Even as we improve Squid code to
eliminate these problematic uses, we should keep these limits until
older installations (that these limits protect) are no longer supported.

In typical environments (e.g., 32-bit int), the new combined mask size
limit is exactly 268'435'454 bytes which is only two bytes smaller than
the ~256MB limit added in 2016 commit 831e953. If an old peer sends a
digest mask with an "extra" byte or two, new code will safely reject it.

An assert() call was improperly used to validate a received cache digest from a cache_peer. Switch to using an invalid value, so that the improper digest is rejected instead.

kinkie · 2026-08-27T06:40:55Z

The relevant call is in peerDigestSetCBlock():783, which calls CalcMaskSize() with a peer-supplied cblock.capacity, triggering the assert.

The new code will cause mismatch between cblock.mask_size and the calculated capacity (we may also add an explicit non-zero check as an additional precaution), causing the rejection of the digest.

No other call to CalcMaskSize( ) uses user input

rousskov

The proposed solution has several conceptual and implementation problems. I will find the time to disclose and fix them. The ball is in my court.

rousskov

I will fix the problems identified in this review. The ball is in my court.

rousskov · 2026-09-01T15:13:10Z

-    uint64_t bitCount = (cap * bpe) + 7;
-    assert(bitCount < INT_MAX); // do not 31-bit overflow later
+    const uint64_t bitCount = (cap * bpe) + 7;
+    if (bitCount >= std::numeric_limits<int>::max())


The proposed code is buggy. For example, CalcMaskSize(2^57 + 1, 128) overflows cap * bpe, resulting in incorrect 135 return value:

cap * bpe = (2^57 + 1) * 128 = (2^57 + 1) * 2^7 = 2^64 + 2^7 = 2^64 + 128;

The above multiplication wraps modulo 2^64, producing just 128, and leading to an incorrect "positive" 135 result rather than overflow detection:

(cap * bpe) + 7 = 128 + 7 = 135;

N.B. Unlike this PR code, the corresponding official code did not even try to overcome 64-bit overflows, but it is also buggy, for the same reason.

rousskov · 2026-09-01T15:21:39Z

-    assert(bitCount < INT_MAX); // do not 31-bit overflow later
+    const uint64_t bitCount = (cap * bpe) + 7;
+    if (bitCount >= std::numeric_limits<int>::max())
+        return 0; // overflow; caller must treat 0 as invalid


Callers may forget to obey this "must". Depending on one's definition of "treat as invalid", it can be argued that at least one and possibly even two existing callers do not really "treat 0 as invalid" already.

In general, please avoid using special valid (from the compiler point of view) values like zeros or empty strings to flag invalid input.

rousskov · 2026-09-01T15:50:03Z


    /// calculate the size of mask required to digest up to
    /// a specified capacity and bitsize.
+    /// \returns 0 when inputs would overflow (invalid).


It also returns 0 in other cases. For example, CalcMaskSize(0, 8) AFAICT. This is one of the dangers with using valid (from the compiler point of view) values like zeros or empty strings to flag invalid input.

P.S. Current code may not have any CalcMaskSize(0, 8) callers, but that assertion does not address this concern because code will change (and because changing such code increases associated caller risks).

Also marked a few unaddressed problems. Addressing them may change this solution!

rousskov · 2026-09-05T22:04:38Z

I will fix the problems identified in this review. The ball is in my court.

Status update: I am almost done with the edits. I hope to push the changes on Tuesday¹.

Monday is a holiday here. I may not be able to work on this. ↩

... with digest mask size calculations and with sender/recipient mask size calculations getting out of sync, causing legacy senders to reject digests generated by this modern/patched code. The only (pre-existing) problem this code does not solve is support for senders and receivers that have different `int` sizes. Fixed code does not assert in such environments. It rejects the digests with a level-0 cache.log message. That is good enough for now. The key in this solution is to limit calculated digest capacity (and derive mask size from that) rather than just focusing on safe mask size calculations derived from raw capacity estimates. This solution works because the recipient uses sender's digest capacity to re-calculate the mask size. If we provide the recipient with safe capacity values and a matching mask size, the legacy recipient should be happy. TODO: * Remove a temporary assertion that duplicates unsafe code. * Polish touched error messages. * Consider reducing diff (and hiding unchanged callers) by avoiding MaskSize() renaming.

It would be nice to keep that assertion, but it requires adding a public CacheDigest::UnsafeMaskSize() method, which is probably too much.

The script mentioned in 2022 commit d816f28 could not handle this case but applied a similar change to a nearby similar debugs().

This addition has no effect on typical 32-bit and 64-bit POSIX systems: SSIZE_MAX is far larger than other limits.

... because the corresponding CacheDigest::init() assertions are (and should always be) satisfied by positive capacity already. We do not have to force mask sizes to be always positive from that point of view.

Existing code still stores the number of bits as `int`. For example, CacheDigestStats::bit_on_count is an `int` and cacheDigestStats() stores bit position `pos` in an `int`.

rousskov

@kinkie, I am done with PR adjustments. Please review, including the new PR title/description, and adjust as needed. Thank you.

rousskov · 2026-09-09T14:57:02Z

+    // 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;


If my math is correct, at 255 bits per entry, it would take about 2.3 million years to reach this limit while storing/adding 1000 new objects every second. Still, it is probably better to have this specific "we can count all bits using uint64_t math" limit than to use std::numeric_limits<uint64_t>::max().

rousskov · 2026-09-09T15:01:25Z

+static uint64_t
+UnsafeMaskSize(const uint64_t cap, const uint8_t bpe)
+{
+    Assure(bpe);


Squid already rejects zero bpe in received digests. We must also reject digest_bits_per_entry 0 configuration directives, but perhaps that should be done in a dedicated PR.

rousskov · 2026-09-09T15:09:53Z

-    // 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) {


This restriction is now R4 inside CacheDigest::CalcMaskSize().

rousskov · 2026-09-09T15:19:31Z

+        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


I used INT_MAX here instead of the usually preferred std::numeric_limits<int>::max() because I wanted to tie R5 to the problematic assertion in legacy code. That assertion is using INT_MAX.

Sure, but then why are you not subtracting 8 from the semantically equivalent test in line 312?

Alex: I used INT_MAX ... to tie R5 to the problematic assertion in legacy code. That assertion is using INT_MAX.

Francesco: Sure, but then why are you not subtracting 8 from the semantically equivalent test in line 312?

Line 312 is R2.

I am not sure I understand the question. AFAICT, either the question is based on a false assumption that R2 and R5 limits are semantically equivalent, or it uses "semantic equivalence" definition that I do not know/share.

R2 and R5 limits/lines target semantically different code and, hence, are semantically distinct.

R2 protects code like int pos = cd->mask_size * 8

R5 protects assert(bitCount < INT_MAX)

Perhaps it would help to explain where that -8 comes from in R5? Here is the math:

bitCount < INT_MAX // legacy assertion (cap * bpe) + 7 < INT_MAX (cap * bpe) + 7 <= INT_MAX - 1 cap * bpe <= INT_MAX - 8 (mask_size * 8 / bpe) * bpe <= INT_MAX - 8 mask_size * 8 <= INT_MAX - 8 mask_size <= (INT_MAX - 8) / 8 // R5 limit

Does this clarify? I doubt we should include the above math in code or PR description, but please add it if you think it should be included.

P.S. If that legacy code asserted mask_size * 8 <= INT_MAX condition instead, then we could merge R2 and R5, but the actual assertion is different, and so we have two different/distinct limits/conditions (R2 and R5).

rousskov · 2026-09-09T15:22:25Z

-               << ").");
+    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 " <<


We can undo all changes in this file, but I think it is best to use a new ERROR message for these cases so that we can tell whether these errors are printed by problematic/legacy code or upgraded one. In a peering hierarchy, it may be tricky to be sure that every instance is running the intended Squid version...

rousskov · 2026-09-09T15:28:32Z

+    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


We can reduce the diff further by using absolute_max instead of safeCap, but I think we should use safeCap instead because we have two sets of variables here, one set for the mask size (safeMaskSizeMax) and one for the digest capacity (safeCapMax and safeCap). absolute_max does not tell the reader which set/object that maximum applies to.

This change fixes cache digest capacity and mask size calculations to improve validation of received cache digest metadata and to prevent generation of cache digests that may trigger problems in both digest-generating instances and digest-receiving peers. This change does not affect Cache Digest population algorithm and exchange protocol. This change preserves compatibility across old and updated peers: * Old Squids accept all digests generated by new code. * New Squids accept non-problematic digests generated by old code. * New Squids safely reject problematic digests generated by old code. The text below details new and updated digest sizing limits. A Squid instance may deal with two sources of Cache Digests: * A digest of the local instance caches. Since 2016 commit 831e953, its mask size is capped at approximately INT_MAX bits. That maximum usually corresponds to a ~256MB digest mask memory allocation. See absolute_max calculation in old storeDigestCalcCap() code. * Digests sent to the instance by its cache_peers. These digests come with peer-set capacity and mask sizes. The received mask size was effectively capped at similar levels (using an assertion) when peerDigestSetCBlock() called old CacheDigest::CalcMaskSize(). The two poorly duplicated limits were tied together by a stale C++ comment. This change removes that duplication, moving limit enforcement from storeDigestCalcCap() into CalcMaskSize() and eliminating the problematic assertion. If Squid receives a cache digest that violates the new limits, that digest will be safely rejected with a level-0 "digest cblock is corrupted or unsupported" ERROR. Also fixed integer overflow in CacheDigest::CalcMaskSize() calculations, addressing an XXX comment correctly added in 2015 commit 5bc5e81 that was incorrectly replaced with an incorrect assertion in 2016 commit 831e953. See new UnsafeMaskSize(). Also explicitly capped digest capacity and mask size to prevent integer overflows where the calculated mask size is used, including uses in old receiver code. See R1-R5 limits in updated CalcMaskSize(). Until all Squids are upgraded, older installations will continue to receive digests generated by upgraded Squids. Even as we improve Squid code to eliminate these problematic uses, we should keep these limits until older installations (that these limits protect) are no longer supported. In typical environments (e.g., 32-bit `int`), the new combined mask size limit is exactly 268'435'454 bytes which is only two bytes smaller than the ~256MB limit added in 2016 commit 831e953. If an old peer sends a digest mask with an "extra" byte or two, new code will safely reject it.

yadij

FWIW; Since C++14 the __int128 type is available. So we should be able to make use of 128-bit variables to avoid overflow in 64-bit math.

rousskov · 2026-09-14T14:00:12Z

FWIW; Since C++14 the __int128 type is available.

What makes you think that? I do not see any 128-bit types explicitly named at https://en.cppreference.com/cpp/types/integer and various sources, including GCC documentation, suggest that such types are compiler extensions (that are not supported on all platforms even by those compilers that do support those extensions).

So we should be able to make use of 128-bit variables to avoid overflow in 64-bit math.

While we are sometimes able to use 128-bit variables to avoid 64-bit overflows during computations, it is most likely a bad idea because 128-bit support is not universal and we would still need to check for overflows when storing/using the result of those computations (in 64-bit or smaller variables). The fundamental problem with overflows cannot be properly addressed by increasing the number of bits. With more bits, that problem footprint becomes smaller, but the problem will not go away until we start using custom types that automatically handle overflows the way we want them to be handled. That correct handling is to be determined and may depend on usage context (e.g., some contexts may want to have special "overflow" state/value (that taints further computations) while others may want to throw C++ exceptions.

All that complexity lies outside this PR scope, of course.

rousskov · 2026-09-14T15:09:18Z

FWIW; Since C++14 the __int128 type is available. ...

... All that complexity lies outside this PR scope, of course.

There is now #2495 where this discussion should continue.

This change fixes cache digest capacity and mask size calculations to improve validation of received cache digest metadata and to prevent generation of cache digests that may trigger problems in both digest-generating instances and digest-receiving peers. This change does not affect Cache Digest population algorithm and exchange protocol. This change preserves compatibility across old and updated peers: * Old Squids accept all digests generated by new code. * New Squids accept non-problematic digests generated by old code. * New Squids safely reject problematic digests generated by old code. The text below details new and updated digest sizing limits. A Squid instance may deal with two sources of Cache Digests: * A digest of the local instance caches. Since 2016 commit 831e953, its mask size is capped at approximately INT_MAX bits. That maximum usually corresponds to a ~256MB digest mask memory allocation. See absolute_max calculation in old storeDigestCalcCap() code. * Digests sent to the instance by its cache_peers. These digests come with peer-set capacity and mask sizes. The received mask size was effectively capped at similar levels (using an assertion) when peerDigestSetCBlock() called old CacheDigest::CalcMaskSize(). The two poorly duplicated limits were tied together by a stale C++ comment. This change removes that duplication, moving limit enforcement from storeDigestCalcCap() into CalcMaskSize() and eliminating the problematic assertion. If Squid receives a cache digest that violates the new limits, that digest will be safely rejected with a level-0 "digest cblock is corrupted or unsupported" ERROR. Also fixed integer overflow in CacheDigest::CalcMaskSize() calculations, addressing an XXX comment correctly added in 2015 commit 5bc5e81 that was incorrectly replaced with an incorrect assertion in 2016 commit 831e953. See new UnsafeMaskSize(). Also explicitly capped digest capacity and mask size to prevent integer overflows where the calculated mask size is used, including uses in old receiver code. See R1-R5 limits in updated CalcMaskSize(). Until all Squids are upgraded, older installations will continue to receive digests generated by upgraded Squids. Even as we improve Squid code to eliminate these problematic uses, we should keep these limits until older installations (that these limits protect) are no longer supported. In typical environments (e.g., 32-bit `int`), the new combined mask size limit is exactly 268'435'454 bytes which is only two bytes smaller than the ~256MB limit added in 2016 commit 831e953. If an old peer sends a digest mask with an "extra" byte or two, new code will safely reject it.

Harden against invalid cache peer digests

7d53bbc

An assert() call was improperly used to validate a received cache digest from a cache_peer. Switch to using an invalid value, so that the improper digest is rejected instead.

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Aug 27, 2026

This comment was marked as resolved.

Sign in to view

kinkie added the backport-to-v7 maintainer has approved these changes for v7 backporting label Aug 27, 2026

squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Aug 27, 2026

rousskov requested changes Aug 27, 2026

View reviewed changes

rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Aug 27, 2026

rousskov requested changes Sep 1, 2026

View reviewed changes

fixup: Do not return compiler-valid zero on invalid input

077262e

Also marked a few unaddressed problems. Addressing them may change this solution!

rousskov added 7 commits September 8, 2026 19:07

fixup: Removed temporary (but correct) assertion

bc4f44c

It would be nice to keep that assertion, but it requires adding a public CacheDigest::UnsafeMaskSize() method, which is probably too much.

fixup: Fixed category of an otherwise modified debugs() messsage

13fb3ae

The script mentioned in 2022 commit d816f28 could not handle this case but applied a similar change to a nearby similar debugs().

fixup: Described new private/helper function

0528c72

Added one more (overlapping) restriction

571630b

This addition has no effect on typical 32-bit and 64-bit POSIX systems: SSIZE_MAX is far larger than other limits.

Removed one paranoid limit

98daf26

... because the corresponding CacheDigest::init() assertions are (and should always be) satisfied by positive capacity already. We do not have to force mask sizes to be always positive from that point of view.

fixup: formatted changes sources

94e9a16

rousskov reviewed Sep 9, 2026

View reviewed changes

Comment thread src/CacheDigest.cc Outdated

rousskov added 2 commits September 9, 2026 09:14

fixup: Undo CalcMaskSize() renaming to reduce diff

34d3a62

fixup: Undo improvements that were justified by now-gone renaming

7227a42

rousskov changed the title ~~Harden against invalid cache peer digests~~ Improve cache digest mask sizing Sep 9, 2026

rousskov added 4 commits September 9, 2026 10:58

fixup: Found more now-out-of-scope improvements to undo

fcdb074

fixup: Restrict "legacy" to old/now-fixed code

d22675a

Existing code still stores the number of bits as `int`. For example, CacheDigestStats::bit_on_count is an `int` and cacheDigestStats() stores bit position `pos` in an `int`.

fixup: Updated stale branch-added comment

cc9ba2d

fixup: Avoid introducing C-specific UINT64_MAX

a64e54f

rousskov approved these changes Sep 9, 2026

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Sep 9, 2026

yadij reviewed Sep 14, 2026

View reviewed changes

rousskov mentioned this pull request Sep 14, 2026

Use 128-bit type to prevent 64-bit overflow in strtoll #2495

Closed

squid-anubis added M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 15, 2026

Conversation

kinkie commented Aug 27, 2026 • edited by rousskov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

kinkie commented Aug 27, 2026

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Sep 5, 2026

Footnotes

Uh oh!

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov commented Sep 14, 2026

Uh oh!

rousskov commented Sep 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinkie commented Aug 27, 2026 •

edited by rousskov

Loading

rousskov Sep 1, 2026 •

edited

Loading