Reject module cross-slot BlockClientOnKeys in cluster mode - #4541
Reject module cross-slot BlockClientOnKeys in cluster mode#4541enjoy-binbin wants to merge 1 commit into
Conversation
If a module blocks a client on keys from different slots, the blocked client is later checked by clientsCronHandleTimeout(), which calls clusterRedirectBlockedClientIfNeeded(). There it hits the slot == c->slot assertion (added in valkey-io#2165), since we do not allow cross-slot commands, and the server aborts. A module can reach this through incorrect use of the API. Even though it is a misuse, it should not be able to crash the server, so reject it in moduleBlockClient() and reply with an error instead of blocking. This adds one more error path that aborts after the blocked handle has been allocated. Such paths must reset bc->client to NULL, otherwise a later VM_UnblockClient() dereferences will crash the server. Only the Lua/MULTI path did this; apply the same to the other paths as well. Signed-off-by: Binbin <binloveplay1314@qq.com>
3b9fdd2 to
8b23d87
Compare
|
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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesModule blocking validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR rejects invalid cross-slot module blocking requests and safely cleans up rejected handles, preventing a server crash without changing valid blocking behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (3 skipped: 2 unsupported, 1 too large.)
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 |
|
DCO is failing for commit |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| } else { | ||
| if (keys) { | ||
| /* In cluster mode, reject blocking on keys from different slots. */ | ||
| if (server.cluster_enabled) { |
There was a problem hiding this comment.
This rejects cross-slot blocking even when a module has set VALKEYMODULE_CLUSTER_FLAG_NO_REDIRECTION. That flag explicitly gives modules freedom to access every key regardless of slot (VM_SetClusterFlags at src/module.c:9889), and the normal cluster gate honors it in getNodeByQuery (src/cluster.c:1060). Keep the same exemption here (and in clusterRedirectBlockedClientIfNeeded, which otherwise still asserts on these clients), so modules implementing their own distribution can continue to block across slots.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4541 +/- ##
============================================
- Coverage 78.83% 78.75% -0.09%
============================================
Files 171 171
Lines 89857 89871 +14
============================================
- Hits 70838 70774 -64
- Misses 19019 19097 +78
🚀 New features to boost your workflow:
|
1 participant
If a module blocks a client on keys from different slots, the blocked
client is later checked by clientsCronHandleTimeout(), which calls
clusterRedirectBlockedClientIfNeeded(). There it hits the slot == c->slot
assertion (added in #2165), since we do not allow cross-slot commands,
and the server aborts.
A module can reach this through incorrect use of the API. Even though
it is a misuse, it should not be able to crash the server, so reject it
in moduleBlockClient() and reply with an error instead of blocking.
This adds one more error path that aborts after the blocked handle has
been allocated. Such paths must reset bc->client to NULL, otherwise a
later VM_UnblockClient() dereferences will crash the server. Only the
Lua/MULTI path did this; apply the same to the other paths as well.