Fix/ready key blocked client uaf 4198#4212
Conversation
Signed-off-by: quanyeyang <quanyemostima@gmail.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)
📝 WalkthroughWalkthroughBlocked-client readiness handling now snapshots queued clients and revalidates them before unblocking. Module commands and a regression test cover killing a successor during reply processing while confirming continued server operation. ChangesBlocked-client safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestClient
participant BlockOnKeysModule
participant ValkeyServer
TestClient->>BlockOnKeysModule: block two clients on the same key
TestClient->>BlockOnKeysModule: signal the key as ready
BlockOnKeysModule->>ValkeyServer: reply callback kills successor client
ValkeyServer->>BlockOnKeysModule: revalidate and process snapshot entry
TestClient->>ValkeyServer: PING
ValkeyServer-->>TestClient: PONG
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4212 +/- ##
============================================
- Coverage 76.81% 76.80% -0.02%
============================================
Files 162 162
Lines 81455 81483 +28
============================================
+ Hits 62570 62581 +11
- Misses 18885 18902 +17
🚀 New features to boost your workflow:
|
ranshid
left a comment
ranshid
left a comment
There was a problem hiding this comment.
Thanks for the fix — the snapshot-by-ID + re-lookup approach looks correct. A couple of scope/direction questions below.
Affected versions / backport scope: The linked issue labels this as 9.1.0-only, but the vulnerable pattern — iterating the blocking-keys list with a plain listIter while the serve path can synchronously free another blocked client — looks a lot older than 9.1 (module block-on-keys + synchronous free reentrancy has been around since at least 7.2). Can you clarify the basis for the 9.1-only claim? If it does reach back to 7.2/8.x, we should plan backports to the active release branches accordingly.
Broader direction (non-blocking): Part of the root cause is that many flows free clients synchronously (CLIENT KILL, evictClients(), COB-limit handling, …), and we keep tripping over iterators/cached pointers that outlive those frees. This snapshot fix is a good local solution, but it may be worth a broader discussion on whether these paths should prefer freeClientAsync() so client teardown always happens at a safe point (beforeSleep) rather than mid-iteration. Raising it here since it's a recurring class of bug, not to block this PR.
| while ((ln = listNext(&li)) && count--) { | ||
| client *receiver = listNodeValue(ln); | ||
| robj *o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NOEFFECTS); | ||
| for (i = 0; i < snapshot_len; i++) { |
There was a problem hiding this comment.
This isn't actually specific to the module reply path. The regular flow — unblockClientOnKey -> processCommandAndResetClient -> processCommand — also re-enters processCommand, which calls evictClients() synchronously, and that does a synchronous freeClient() on other memory-heavy clients. With maxmemory-clients set, that can tear down another client still blocked on this same key (the successor node the old listIter had cached), so a plain BLPOP/BLMOVE could hit the same UAF with no module involved. The good news is the ID-snapshot + re-lookup here covers that case too, since we no longer retain a listNode*/client* across the serve call. Might be worth calling this out so the general nature of the fix is clear.
@ranshid On affected versions: the 9.1.0-only wording came from the original report. On the non-module path: good catch. unblockClientOnKey → processCommand → On freeClientAsync(): agreed this is a recurring class of bugs. |
Signed-off-by: quanyeyang <quanyemostima@gmail.com> Snapshot client pointers and re-check live blocking-list membership instead of lookupClientByID(), which misses module fake clients and broke async RM_Call BLPOP (fire-and-forget).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/blocked.c`:
- Around line 653-675: Optimize the validation loop around
clientStillInBlockingKeysList by snapshotting each client’s ID alongside its
pointer. For normal clients, validate liveness and identity with
lookupClientByID() in O(1), while retaining the existing linear
clientStillInBlockingKeysList fallback for module fake clients or clients whose
pointers were freed; preserve the initial-count processing limit and existing
blocking-list membership behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: quanyeyang <quanyemostima@gmail.com> Snapshot client pointer and id; re-check via lookupClientByID() for registered clients, and fall back to a live blocking-list scan for RM_Call fake clients missing from clients_index.
| * invalidating a listIter's cached successor. Re-check live list | ||
| * membership before serving — also covers RM_Call fake clients that | ||
| * are absent from clients_index. */ | ||
| snapshot = zmalloc(sizeof(*snapshot) * count); |
There was a problem hiding this comment.
dereferencing a pointer that was never assigned an initial value is UB
sizeof(*snapshot) * count should be (sizeof(client*) + sizeof(uint64_t)) * count
There was a problem hiding this comment.
Thanks for taking a look. sizeof(*snapshot) does not evaluate/
dereference snapshot — sizeof on a non-VLA operand is unevaluated
and this is the usual C idiom for malloc(sizeof(*p)).
Using sizeof(client*) + sizeof(uint64_t) would also be incorrect
where the struct has padding (e.g. 32-bit), and could under-allocate.
In C23 N3220: §6.5.4.4 The sizeof and alignof operators，paragraph 2
The sizeof operator yields the size (in bytes) of its operand, which may be an expression or the
parenthesized name of a type. The size is determined from the type of the operand. The result
is an integer. If the type of the operand is a variable length array type, the operand is evaluated;
otherwise, the operand is not evaluated and the result is an integer constant.
And here is an example:
There was a problem hiding this comment.
you're right, thanks for the useful info
AlisinaDevelo
left a comment
AlisinaDevelo
left a comment
There was a problem hiding this comment.
Not a maintainer, just read through it carefully since it is a security fix. The snapshot-{client*, id} + re-lookup approach looks right to me, and the regression test reproduces the successor-free case well. Built it and runtest-moduleapi --single unit/moduleapi/blockonkeys passes here.
One thing I could not fully convince myself about, on the fallback branch:
if (lookupClientByID(snapshot[i].id) == receiver) {
if (!clientStillBlockedOnReadyKey(receiver, rl)) continue;
} else if (!clientStillInBlockingKeysList(rl, receiver)) {
continue;
}The fallback (clientStillInBlockingKeysList) matches purely on the client\* pointer and ignores the snapshotted id. It is only reached when lookupClientByID(id) != receiver, i.e. for clients absent from clients_index — which is exactly the fake/module clients (createClient only does linkClient when conn != NULL).
But fake clients are not heap-freed on release — moduleReleaseTempClient recycles the same client\* back into moduleTempClients and a later RM_Call hands the same address out again with a fresh id. So a pooled address can be alive again as a different logical client. If a pooled fake client at address A is snapshotted (id X), released mid-serve, then re-acquired as a different fake client (id Y) that ends up on this key’s blocking list, the pointer scan would match A and serve it through slot X — bypassing the stronger id-confirmed clientStillBlockedOnReadyKey check.
Two honest possibilities, and I do not know the module internals well enough to say which:
- A pooled fake client can never sit on a
blocking_keyslist (RM_Call temp clients do not block on keys), in which case the fallback never positively matches a live client and could be simplified / commented as such; or - It can, in which case the fallback probably wants to confirm the id too, not just the pointer.
Either way it might be worth a comment pinning down when the fallback is expected to match a live client. Apologies if this is already covered by an invariant I have missed.
Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Thanks for the careful read — this was a fair concern. You’re right that the fallback was pointer-only, and lookupClientByID(id) != receiver is not only “fake client”: it also covers a real client that was freed, in which case jemalloc can reuse the same address for a new client that later blocks on the same key. Matching only client* could then serve the wrong logical client. One small clarification on module temp clients: moduleAllocTempClient() recycles the pooled object without assigning a new c->id, so the “same address, fresh id Y” pool ABA is less common than the heap-reuse case for real clients. Still, the fallback needed to be stricter. I tightened it to: live-list membership first (so we don’t deref a freed client), then receiver->id == snapshotted id, then clientStillBlockedOnReadyKey(). That rejects address reuse after freeClient(), and keeps the RM_Call fake-client path working. Added a short comment on when that fallback is expected to match. |
4 participants
Summary
handleClientsBlockedOnKey()when serving blocked clients.listItercaches the successorlistNode*, while the serve path can synchronouslyfreeClient()another client still blocked on the same key (module reply +CLIENT KILL, and alsounblockClientOnKey→processCommand→evictClients()withmaxmemory-clients).lookupClientByID()and skip clients that are no longer blocked on the key.Reported by @gff-cw in #4198.
Affected versions / backport
The original report mentioned 9.1.0, but the vulnerable pattern appears older than 9.1. Happy to help with backports to active release branches if desired.
Test plan
CLIENT KILLs a successor blocked client./runtest --single unit/moduleapi/blockonkeysFixes #4198