Fix signed integer overflow in refreshable materialized view RANDOMIZE FOR - #113673
Fix signed integer overflow in refreshable materialized view RANDOMIZE FOR#113673groeneai wants to merge 2 commits into
RANDOMIZE FOR#113673Conversation
RANDOMIZE FOR accepts an unbounded UInt64 and the random factor is a signed Int64, but RefreshSchedule::addRandomSpread computed the random offset in double and then pushed it through three unchecked domains in one expression: the double to Int64 conversion, the milliseconds to microseconds widening, and the time_point addition. system_clock::duration is microseconds, so a representable milliseconds value is not automatically safe, and each of the three has its own threshold and its own frame. Compute the offset in Int128, the narrowest width that holds the widest possible product of the two unbounded operands (UInt64_MAX * Int64_MIN fits with 2^63 to spare; after the division the final sum has a factor of 2000), then saturate the result strictly inside the time_point range. Strictly inside matters because time_point::max() is the "no refresh scheduled" sentinel; producing it would make a view read as waiting indefinitely. Operand order is load-bearing: dividing before scaling back up is what keeps the product inside Int128, whereas the naive order overflows it. The same change covers the coordination znode, which deserializes the random factor with no range check, so any Int64 a znode holds reaches this function rather than only the range the scheduler draws from. This is not exactly behaviour-preserving. The intermediate product leaves the exact range of double above a spread of about five hours, and above about 100 days that rounding changes the result the old path returned; the Int128 form returns the exact value instead, a difference of a few milliseconds (measured bound 4 ms) in a deliberately randomized jitter. The changelog entry says so rather than describing the change as a no-op. Two adjacent overflows in the same subsystem are deliberately left alone and tracked separately: CalendarTimeInterval's own arithmetic, whose months field wraps, and the delay this schedule hands to BackgroundSchedulePool. The stateless test has one arm per domain plus a control with an ordinary spread, and a sign-independent oracle, since the random factor is drawn over both signs and a wide window legitimately yields a past-due instant. Only a positive draw overflows in the third domain, so that arm is probabilistic and uses enough views to make a miss negligible; the deterministic pin for that domain is in the gtest, which drives the addition to both ends of the range with a fixed randomness. The gtest also covers what the stateless test cannot see: two of the guards fail inside the wide-integer arithmetic, where the sanitizer emits no check and the result is silently wrong rather than aborting.
Internal second-model review (click to expand)Two review rounds, each with an independent second-model pass over the whole change. Six findings
|
Pre-PR validation gate (click to expand)
Five mutants of the fix were built, each with its own build id, and each reddens at least one test: Session id: cron:clickhouse-review-slot-48:20260806-123800 |
|
cc @al13n321 @tuanpach, could you review this? RANDOMIZE FOR takes an unbounded UInt64 and the random factor is a signed Int64, so addRandomSpread had three unchecked domains in one expression, the double to Int64 conversion, the milliseconds to microseconds widening and the time_point addition, each of which aborts a sanitizer build; the offset is now computed in Int128 and saturated strictly inside the time_point range, which also corrects a few-millisecond rounding error for windows wider than about 100 days. |
PedroTadim
added
the
can be tested
|
Workflow [PR], commit [5ce7325] Summary: ❌
AI ReviewSummaryThis PR replaces the Final Verdict✅ No remaining review findings. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 52/52 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)No arm_release build profile data for commit 5ce7325 - the build was skipped, reused from cache, or predates profile upload. |
Every arm of 04757 passed a window expressed in seconds, so `months` was zero in all of them and the months term of CalendarTimeInterval::minSeconds was never exercised. Add a YEAR and a MONTH arm to the stateless test, and months-only cases to the unit test. The stateless arms are deterministic rather than probabilistic like arm C: escaping the milliseconds-to-microseconds widening pre-fix needs |randomness| <= 1015 for the YEAR arm and <= 6044 for the MONTH arm, out of the 1e9 the scheduler draws. Measured on a pristine ASan+UBSan master build, each arm run alone aborts at RefreshSchedule.cpp:73:51, and both are clean on the fixed build. The unit cases are what pin the calendar term itself: with only the existing seconds-based cases, replacing spread.minSeconds with spread.seconds inside addRandomSpread left every assertion passing in both tests. Their windows (1, 12 and 24 months) do not wrap in minSeconds, so the expected offsets are exact. Verified against that mutant: it fails only the new unit case. Also add the atomic-database tag. Every view here is non-APPEND with a Memory target, and in a Replicated database StorageMaterializedView forces refresh_coordinated and then refuses that combination, so the test cannot pass under --replicated-database. The runner only acts on the tag when the database engine is not Atomic, so no passing run is skipped, and the sibling refreshable-view tests already carry it. The wrap in the unsigned calendar conversion itself is pre-existing and out of scope: CalendarTimeInterval.cpp is byte-identical between this branch and master, and an input whose wrap does collapse the window to a small value schedules identically on both.
CI finish ledger - 5ce7325Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task CI is fully finished on this head: 175/175 check-runs completed, 157 success / 17 skipped,
Not caused by this PR. The diff is confined to The three failing rows are one server abort, not three defects: the Session id: cron:our-pr-ci-monitor:20260807-153000 |
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix undefined behavior when a refreshable materialized view uses a large
RANDOMIZE FORwindow.RefreshSchedule::addRandomSpreadcomputed the random offset indoubleand then narrowed it toInt64milliseconds, widened those to microseconds and added them to atime_point, none of which was range-checked, so a sufficiently wide window aborted the server on a sanitizer build and produced an out-of-range next-refresh time otherwise. The offset is now computed inInt128and saturated inside the representable range. This also removes a rounding error of up to a few milliseconds in the jitter for windows wider than about 100 days, where thedoublecomputation no longer returned the exact value.Description
RANDOMIZE FORaccepts an unboundedUInt64and the random factor is a signedInt64, soaddRandomSpreadhad three unchecked domains in one expression: thedoubletoInt64conversion,the milliseconds to microseconds widening (
system_clock::durationis microseconds, so arepresentable millisecond value is not automatically safe), and the
time_pointaddition. Each isreachable with ordinary SQL (the feature is GA) and aborts a sanitizer build, which uses
-fno-sanitize-recover=all.The fix computes the offset in
Int128, the narrowest width holding the widest possible product ofthe two operands (
UInt64_MAX * Int64_MINfits with 2^63 to spare; the final sum with a factor of2000), then saturates strictly inside the
time_pointrange. Strictly inside, becausetime_point::max()is the "no refresh scheduled" sentinel and producing it would make a view waitindefinitely. Dividing before scaling back up is what keeps the product inside
Int128.This also covers the coordination znode, which deserializes the random factor with no range check, so
any
Int64it holds reaches this function, not just the range the scheduler draws.Validation, on an ASan+UBSan build with the CI job's flags: three arms, one per domain, plus a
YEARand a
MONTHarm reaching the offset through a calendar-unit window, and an ordinary control.Each arm aborts on unpatched master, also alone, and passes after the change. Six mutants of the fix
each redden a test. The new test passes 20 consecutive runs; three siblings pass 50 runs each, the
long-tagged fourth 5.Two adjacent overflows in the same subsystem are out of scope and tracked separately:
CalendarTimeInterval's arithmetic, and the delay this schedule hands toBackgroundSchedulePool.The retry-backoff half of this family is
#113007. No related open issue found.