Fix signed overflow in vset bucket-timestamp rounding - #4312
Conversation
Hash-field expiry timestamps inside the top 8192ms window below 2^63 overflow the vset bucket-timestamp math (get_bucket_ts / get_max_bucket_ts round up via `(expiry & ~(INTERVAL-1)) + INTERVAL`, which wraps signed long long). The overflowed value becomes a negative RAX bucket key; because RAX keys compare as unsigned big-endian bytes it sorts after every real timestamp, and forcing a full vector bucket to split trips assert(target_bucket_ts < bucket_ts) in splitBucketIfPossible (vset.c:1151), aborting the server. VsetTest.TestVsetLargeExpiryBucketOverflow reproduces the crash: it fills a vector bucket with 126 entries at ts A (2^63-8192) and 1 at ts B (a later 16ms sub-window in the same 8192ms window), then adds a 128th entry to force the vector->rax split. It also adds a tiny-TTL entry last and asserts it is scanned first, confirming the huge-TTL entries no longer invert bucket scan order. The test currently fails (aborts). Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
get_bucket_ts() and get_max_bucket_ts() rounded an expiry up to the end of its 16ms / 8192ms window via `(expiry & ~(INTERVAL-1)) + INTERVAL`. For any hash-field expiry inside the top window below 2^63 this addition overflowed signed long long and wrapped to a negative value. That value became a RAX bucket key; because RAX keys are compared as unsigned big-endian bytes, the negative key sorted after every real timestamp. When a full vector bucket was then forced to split, findSplitPosition() returned a positive target while the poisoned bucket_ts was negative (LLONG_MIN), tripping assert(target_bucket_ts < bucket_ts) in splitBucketIfPossible (vset.c) and aborting the server. This was reachable by any client via HPEXPIREAT/HEXPIREAT with a large absolute timestamp (HSET + HPEXPIREAT sequence), i.e. an authenticated crash. Make both helpers saturate at LLONG_MAX instead of overflowing, so a bucket timestamp can never become negative and poison key ordering. This keeps the accepted expiry range consistent with top-level key TTLs. Verified by the regression tests added in the previous commit: VsetTest.TestVsetLargeExpiryBucketOverflow and the HPEXPIREAT near-max timestamp test in hashexpire.tcl now pass. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change saturates bucket timestamps at ChangesExpiry bucket overflow handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the saturating rounding change. The direction is right — a negative bucket_ts is unambiguously broken, and the unit test does reproduce the split assert. One residual hole at the very top of the range: saturating at LLONG_MAX makes bucket_ts == expiry for expiry == LLONG_MAX, which findBucket()'s strict > seek can never resolve. Details inline, including what I verified against the real rax.c.
| static inline long long get_max_bucket_ts(long long expiry) { | ||
| return (expiry & ~(VOLATILESET_BUCKET_INTERVAL_MAX - 1LL)) + VOLATILESET_BUCKET_INTERVAL_MAX; | ||
| long long aligned = expiry & ~(VOLATILESET_BUCKET_INTERVAL_MAX - 1LL); | ||
| if (aligned > LLONG_MAX - VOLATILESET_BUCKET_INTERVAL_MAX) return LLONG_MAX; |
There was a problem hiding this comment.
Saturating at LLONG_MAX breaks the "bucket_ts is the exclusive end of the window" invariant documented at vset.c:87 (expiry < bucket_ts) at the top of the range: get_max_bucket_ts(LLONG_MAX) == LLONG_MAX and get_bucket_ts(LLONG_MAX) == LLONG_MAX, so an entry whose expiry is exactly LLONG_MAX gets a bucket key equal to its own expiry. findBucket() resolves a bucket with raxSeek(&iter, ">", key) over encodeExpiryKey(expiry) (vset.c:1086), which is strictly greater (rax.c:1509 sets gt without eq), so that bucket can never be found again for that entry.
Two consequences, both reachable with HPEXPIREAT k 9223372036854775807 FIELDS ... on a hash holding 128+ volatile fields (convertExpireArgumentToUnixTime accepts exactly LLONG_MAX when basetime == 0, expire.c:740, and 128 volatile fields is what puts the vset in RAX encoding):
insertToBucket_RAX()seesVSET_BUCKET_NONEandraxInsert()s a fresh SINGLE bucket under the same key at vset.c:1257.raxInsertoverwrites andoldis passed as NULL, so the previous bucket pointer is dropped — the 127-entrypVectordemoted into the rax at vset.c:1795 is leaked and its entries silently disappear from the set.- The next
HDEL/HSETEX/HPERSISTon one of those orphaned fields hitsassert(bucket != VSET_NONE_BUCKET_PTR)inremoveFromBucket_RAX()(vset.c:1455) — thatassertisserverAssertviaserverassert.h, so it aborts in release builds too. On the update pathvsetBucketUpdateEntry_RAX()returns NONE (vset.c:1970), which firesserverAssert(vsetUpdateEntry(...) == 1)at t_hash.c:142.
I confirmed this by linking these two helpers plus encodeExpiryKey/findBucket verbatim against the real src/rax.c: on the PR head, findBucket(LLONG_MAX) returns VSET_BUCKET_NONE while a bucket keyed LLONG_MAX is present, and the follow-up raxInsert returns 0 with old = the previous bucket. On unstable the same call resolves the bucket, because the wrapped LLONG_MIN key encodes as 0x8000..., which does sort strictly after 0x7FFF.... So this specific expiry regresses from working to crashing.
Since no long long bucket_ts can be strictly greater than LLONG_MAX, saturation alone cannot preserve the invariant. Either keep the largest expiry the vset can store strictly below LLONG_MAX (reject/cap it before it reaches vsetAddEntry, so RDB/RESTORE/replication are covered too), or teach findBucket() to resolve the saturated top bucket explicitly — exact-match on the LLONG_MAX key when get_max_bucket_ts(expiry) == LLONG_MAX and the > seek came up empty. Either way it is worth extending TestVsetLargeExpiryBucketOverflow to LLONG_MAX itself; the chosen A/B values are both strictly below it, so this boundary is untested.
There was a problem hiding this comment.
agree. handling the LLONG_MAX ttl is required. since preventing it is a breaking change, I prefer to fix it correctly
| * 128th tips the VECTOR over its size limit and forces the vector->rax | ||
| * conversion + split that overflows. Aborts without the fix; with the fix | ||
| * (saturating get_bucket_ts/get_max_bucket_ts) it succeeds. */ | ||
| TEST_F(VsetTest, TestVsetLargeExpiryBucketOverflow) { |
There was a problem hiding this comment.
The commit message for e164e4e states this was "Verified by ... the HPEXPIREAT near-max timestamp test in hashexpire.tcl", but the PR changes only src/vset.c and src/unit/test_vset.cpp, and tests/unit/hashexpire.tcl has no near-max-timestamp test at this ref. Either drop that line from the message or add the test.
Given the PR describes this as an authenticated DoS reachable with plain HSET + HPEXPIREAT, the command-level sequence from the description is worth adding to tests/unit/hashexpire.tcl alongside the unit test — that is the path that guards the reachable repro end to end (argument validation, the 128-volatile-field threshold that switches the vset to RAX encoding, and the subsequent HDEL/HGETALL on those fields), which the vset-level test does not exercise.
There was a problem hiding this comment.
@valkey-review-bot I wanted to reduce the load on our tcl tests, by only focusing on unittest addition which is focused on the problem in this specific data structure implementation.
The previous commit made get_bucket_ts()/get_max_bucket_ts() saturate at
LLONG_MAX to stop the signed overflow that poisoned RAX bucket keys. That
fixes every expiry strictly below LLONG_MAX, but it leaves one broken
boundary: for expiry == LLONG_MAX both helpers return LLONG_MAX, so the
entry is stored under a bucket key equal to its own expiry. This violates
the exclusive-end invariant (entries must satisfy expiry < bucket_ts, see
the header comment) at the single value where it is unavoidable -- no
long long bucket_ts can be strictly greater than LLONG_MAX.
findBucket() resolves an entry's bucket with raxSeek(">", encode(expiry)),
a strictly-greater seek. When the bucket key equals encode(LLONG_MAX) the
seek can never match it, so the LLONG_MAX bucket becomes unfindable:
- A second LLONG_MAX insert takes the "no bucket" path and raxInsert()s a
fresh bucket under the same key with old = NULL, silently overwriting
(and leaking) the bucket already stored there and dropping its entries.
- A subsequent HDEL/HSETEX/HPERSIST on one of those entries hits
serverAssert(bucket != VSET_NONE_BUCKET_PTR) in removeFromBucket_RAX(),
aborting the server in release builds too.
expiry == LLONG_MAX is a reachable input: convertExpireArgumentToUnixTime
accepts it (HPEXPIREAT k 9223372036854775807 ...), and RDB load / RESTORE
/ replication feed it straight into vsetAddEntry without command
validation. On a hash with 128+ volatile fields (RAX encoding) this
regressed from working (on unstable the wrapped negative key happened to
sort last) to data loss + crash.
Fix findBucket() to treat the saturated terminal window as inclusive:
when the ">" seek finds nothing and get_max_bucket_ts(expiry) == LLONG_MAX,
probe the LLONG_MAX key directly with an "=" seek. The probe is scoped so
it effectively fires only for expiry == LLONG_MAX -- any expiry below it
still resolves the LLONG_MAX bucket via the existing ">" seek. This keeps
the fix at the same vset chokepoint, covering commands, RDB, RESTORE, and
replication uniformly, without mutating stored TTLs.
Tests:
- Extend VsetTest.TestVsetLargeExpiryBucketOverflow with two LLONG_MAX
entries and a full last-to-first drain: asserts no entry is lost (the
overwrite would drop one) and that every expiry -- LLONG_MAX included --
is removable, leaving the set empty.
- Re-add tests/unit/hashexpire.tcl integration coverage: the near-2^63
HPEXPIREAT vector->rax split flow, and an HPEXPIREAT-at-LLONG_MAX flow
that HDELs a field (the command path that aborts without this fix).
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
ASSERT_EQ expands to an if/else, so the unbraced `if (count == 0) ASSERT_EQ(...)` in TestVsetLargeExpiryBucketOverflow is a dangling else. GCC with -Wall -Werror rejects it: test_vset.cpp: error: suggest explicit braces to avoid ambiguous 'else' [-Werror=dangling-else] This broke the `make all-with-unit-tests` step on CI (Apple clang does not flag it, which is why it slipped through local builds), cascading into the build/32bit/sanitizer/compatibility job failures. Wrap the guarded ASSERT_EQ in explicit braces. Verified with g++-15 -Wall -Wextra -Werror: the unbraced form reproduces the dangling-else error and the braced form compiles clean. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4312 +/- ##
============================================
- Coverage 76.87% 76.85% -0.03%
============================================
Files 162 162
Lines 81497 81551 +54
============================================
+ Hits 62653 62675 +22
- Misses 18844 18876 +32
🚀 New features to boost your workflow:
|
clang-format-check requires the braced guard body on its own line:
if (count == 0) {
ASSERT_EQ(mockGetExpiry(entry), SMALL);
}
Reformat the dangling-else guard accordingly (still braced, so no
dangling-else regression). clang-format now reports no diff for
src/vset.c or src/unit/test_vset.cpp.
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
1 participant
vsetgroups hash fields into time buckets keyed by the end of their expiry window. It computes that key by rounding the expiry up to the next window boundary:For any expiry in the top 8192ms window below 2⁶³, that addition overflows signed
long longand wraps to a negative value. The negative value then becomes a RAX bucket key. Because RAX compares keys as unsigned big-endian bytes, this key sorts after every legitimate timestamp — silently corrupting bucket order. When a full vector bucket is later forced to split,findSplitPosition()returns a normal positive target while the poisoned key is negative, so the invariant check fails and the server aborts:How it manifests in a real flow
HPEXPIREATaccepts any absolute timestamp up toLLONG_MAX, so a client can trigger this with ordinary commands — no debug hooks or crafted state:The 128th field pushes the bucket past the vector size limit and forces the vector→rax split, where the overflowed negative
bucket_tsfails the assertion and takes the server down. Any authenticated client with write access to a hash can do this, so it is a denial-of-service, not just a debug-build assert.The same corruption also reaches
vsetthrough paths that skip command validation — RDB load,RESTORE, and replication full-sync all feed the field expiry straight intovsetAddEntry. A fix at the command layer alone would not cover them.Fix
Make
get_bucket_ts()andget_max_bucket_ts()saturate atLLONG_MAXinstead of overflowing, so a bucket key can never go negative and invert ordering. The fix lives in thevsetmath — the single chokepoint every ingestion path funnels through — so it protects live commands, RDB/RESTORE, and replication alike, and keeps the accepted expiry range consistent with top-level key TTLs.Testing
VsetTest.TestVsetLargeExpiryBucketOverflow, which reproduces the exact split scenario and also verifies a small-TTL entry still scans before the huge-TTL bucket. It aborts on the current code and passes with the fix.VsetTestsuite and the hash-field-expiry integration tests pass.