Skip to content

fix(cache): clamp capacityInGB so the water marks cannot overflow - #1730

Merged
lihuiba merged 2 commits into
alibaba:mainfrom
xiaoyang-hhh:fix/cache-capacity-watermark-overflow
Nov 6, 2026
Merged

lihuiba merged 2 commits into
alibaba:mainfrom
xiaoyang-hhh:fix/cache-capacity-watermark-overflow

Conversation

@xiaoyang-hhh

@xiaoyang-hhh xiaoyang-hhh commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

FileCachePool's constructor derives three values from capacityInGB with no bound check:

int64_t capacityInBytes = capacityInGB_ * kGB;
waterMark_ = calcWaterMark(capacityInBytes, kMaxFreeSpace);
// keep this relation : waterMark < riskMark < capacity
riskMark_  = std::max(capacityInBytes - kEvictionMark,
                      (static_cast<int64_t>(waterMark_) + capacityInBytes) >> 1);

A large capacityInGB — a unit typo, or a caller passing bytes where GB is expected — wraps all three, and nothing reports it:

capacityInGB capacityInBytes waterMark_ riskMark_
2^33 INT64_MIN huge 9223372031486066688
2^34 0 0 0
UINT64_MAX huge -54760833024 …

2^34 is the worst case, precisely because it is silent: every mark collapses to zero, so totalUsed_ >= waterMark_ is always true and every eviction round tries to empty the whole cache, while riskMark_ == 0 keeps the write path permanently in the risk zone. A negative riskMark_ instead makes writes fail with ENOSPC right away.

Fix

Clamp capacityInGB_ in the constructor and warn.

1 PiB is a round number far below the tightest of the three limits — calcWaterMark multiplies capacity by kWaterMarkRatio in uint64_t before the * 0.01 scaling, so that multiply wraps above 190,887,435 GB. (For comparison: the waterMark_ + capacityInBytes sum overflows int64_t at 4,521,018,207 GB, and capacityInGB * kGB goes negative at 2^33 - 1 GB.) The warning only reports the clamp and its overflow-prevention purpose. Capacity-based eviction remains enabled at the clamped limit, and diskAvailInBytes continues to provide an independent disk-space floor.

capacityInGB == 0 means "zero capacity, never cache", so the clamp only fires above the bound and leaves 0 alone (RoCachedFs.BasicCacheFull depends on that).

Test

CachePool.huge_capacity_does_not_overflow_water_marks asserts 0 < waterMark_ < riskMark_ < capacity for 2^33, 2^34 and UINT64_MAX, plus that the writes the wrecked marks used to reject with ENOSPC now succeed, and that capacityInGB == 0 still yields all-zero marks.

Verified red-green. With the clamp disabled the test fails at all three capacities with exactly the values in the table above:

cache_test.cpp:840: Failure
Expected: (T::risk_mark(pool)) < (capacity), actual: 9223372031486066688 vs -9223372036854775808
capacityInGB=8589934592
cache_test.cpp:837: Failure
Expected: ((int64_t)T::water_mark(pool)) > (0), actual: 0 vs 0
capacityInGB=17179869184
cache_test.cpp:837: Failure
Expected: ((int64_t)T::water_mark(pool)) > (0), actual: -54760833024 vs 0
capacityInGB=18446744073709551615

Full cache_test suite on this branch: 23 tests from 4 test cases ran. [ PASSED ] 23 tests., 0 failures.

FileCachePool's constructor derives three values from capacityInGB with no
bound check:

    int64_t capacityInBytes = capacityInGB_ * kGB;
    waterMark_ = calcWaterMark(capacityInBytes, kMaxFreeSpace);
    riskMark_  = std::max(capacityInBytes - kEvictionMark,
                          (waterMark_ + capacityInBytes) >> 1);

A large capacityInGB -- a unit typo, or a caller passing bytes where GB is
expected -- wraps all three, and nothing reports it:

  capacityInGB   capacityInBytes   waterMark_     riskMark_
  2^33           INT64_MIN         huge           9223372031486066688
  2^34           0                 0              0
  UINT64_MAX     huge              -54760833024   ...

2^34 is the worst case: every mark collapses to zero, so
totalUsed_ >= waterMark_ is always true and every eviction round tries to
empty the whole cache, while riskMark_ == 0 keeps the write path permanently
in the risk zone. A negative riskMark_ instead makes writes fail with ENOSPC
right away.

Clamp capacityInGB_ in the constructor and warn. 1 PiB is a round number far
below the tightest of the three limits -- calcWaterMark multiplies capacity
by kWaterMarkRatio in uint64_t before scaling, which wraps above
190,887,435 GB. Above 1 PiB capacity-based eviction is meaningless anyway,
and diskAvailInBytes still governs.

capacityInGB == 0 means "zero capacity, never cache", so the clamp only
fires above the bound and leaves 0 alone.

The new test asserts 0 < waterMark_ < riskMark_ < capacity for 2^33, 2^34
and UINT64_MAX, plus that the writes the wrecked marks used to reject with
ENOSPC now succeed. Verified red-green: with the clamp disabled it fails at
all three capacities with exactly the values tabulated above.
Comment thread fs/cache/full_file_cache/cache_pool.cpp Resolved
@lihuiba
lihuiba merged commit ef23918 into alibaba:main Nov 6, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants