Clean up lookahead-related code#3
Conversation
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
📝 WalkthroughWalkthroughThe changes refactor cross-slot detection in a Redis cluster by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db.c (1)
3185-3204:⚠️ Potential issue | 🟠 MajorInitialize
*slotwhen no keys are found.When
num_keys <= 0,*slotis left unchanged, so callers can observe a stale value. The contract says the slot is returned via*slot, so it should be explicitly set toINVALID_CLUSTER_SLOTon the no‑key/error paths.✅ Suggested fix
- if (num_keys > 0) { - *slot = extractSlotFromKeysResult(argv, result); - } + *slot = INVALID_CLUSTER_SLOT; + if (num_keys > 0) { + *slot = extractSlotFromKeysResult(argv, result); + }
🤖 Fix all issues with AI agents
In `@src/cluster.c`:
- Around line 1112-1122: The for-loop over keys_result is off-by-one and can
access beyond the arrays; change the loop condition in the block that uses
keys_result->numkeys, slot_buffer and keys_result->keys so it iterates j from 0
up to but not including keys_result->numkeys (i.e. use j < keys_result->numkeys)
to avoid reading argv/keys_result->keys[numkeys] or writing
slot_buffer[numkeys]; ensure the rest of the logic using this_key, this_slot,
first_slot, INVALID_CLUSTER_SLOT and returning CLUSTER_CROSSSLOT is unchanged.
In `@src/db.c`:
- Around line 428-439: The loop in extractSlotFromKeysResult (the code iterating
with j <= keys_result->numkeys over the keys_result buffer) is off-by-one and
can read/write past the allocated buffer; update that loop condition to use j <
keys_result->numkeys (and any equivalent <= uses) so you only iterate up to
numkeys-1, and verify any buffers sized with numkeys are accessed with the
corrected index bounds; after fixing extractSlotFromKeysResult, ensure
getSlotFromCommand continues to call extractSlotFromKeysResult unchanged.
🧹 Nitpick comments (1)
src/cluster_slot_stats.c (1)
240-244: Update line 243 to useINVALID_CLUSTER_SLOTfor consistency.The raw value
-1should be replaced with theINVALID_CLUSTER_SLOTconstant to match the pattern used elsewhere in this file (lines 140, 226, 252).♻️ Proposed fix
void clusterSlotStatsInvalidateSlotIfApplicable(scriptRunCtx *ctx) { if (!(ctx->flags & SCRIPT_ALLOW_CROSS_SLOT)) return; - ctx->original_client->slot = -1; + ctx->original_client->slot = INVALID_CLUSTER_SLOT; }
| for (int j = 0; j <= keys_result->numkeys; j++) { | ||
| robj *this_key = argv[keys_result->keys[j].pos]; | ||
| int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr)); | ||
| slot_buffer[j] = this_slot; | ||
|
|
||
| if (first_slot == INVALID_CLUSTER_SLOT) | ||
| first_slot = this_slot; | ||
| else if (first_slot != this_slot) { | ||
| return INVALID_CLUSTER_SLOT; | ||
| free(slot_buffer); | ||
| return CLUSTER_CROSSSLOT; | ||
| } |
There was a problem hiding this comment.
Fix off-by-one loop to avoid OOB access.
Line 1112 iterates j <= keys_result->numkeys, which reads keys_result->keys[numkeys] and writes slot_buffer[numkeys] out of bounds. This can corrupt memory or crash.
🛠️ Proposed fix
- for (int j = 0; j <= keys_result->numkeys; j++) {
+ for (int j = 0; j < keys_result->numkeys; j++) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (int j = 0; j <= keys_result->numkeys; j++) { | |
| robj *this_key = argv[keys_result->keys[j].pos]; | |
| int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr)); | |
| slot_buffer[j] = this_slot; | |
| if (first_slot == INVALID_CLUSTER_SLOT) | |
| first_slot = this_slot; | |
| else if (first_slot != this_slot) { | |
| return INVALID_CLUSTER_SLOT; | |
| free(slot_buffer); | |
| return CLUSTER_CROSSSLOT; | |
| } | |
| for (int j = 0; j < keys_result->numkeys; j++) { | |
| robj *this_key = argv[keys_result->keys[j].pos]; | |
| int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr)); | |
| slot_buffer[j] = this_slot; | |
| if (first_slot == INVALID_CLUSTER_SLOT) | |
| first_slot = this_slot; | |
| else if (first_slot != this_slot) { | |
| free(slot_buffer); | |
| return CLUSTER_CROSSSLOT; | |
| } |
🤖 Prompt for AI Agents
In `@src/cluster.c` around lines 1112 - 1122, The for-loop over keys_result is
off-by-one and can access beyond the arrays; change the loop condition in the
block that uses keys_result->numkeys, slot_buffer and keys_result->keys so it
iterates j from 0 up to but not including keys_result->numkeys (i.e. use j <
keys_result->numkeys) to avoid reading argv/keys_result->keys[numkeys] or
writing slot_buffer[numkeys]; ensure the rest of the logic using this_key,
this_slot, first_slot, INVALID_CLUSTER_SLOT and returning CLUSTER_CROSSSLOT is
unchanged.
| /* Return the slot of the key in the command. | ||
| * GETSLOT_NOKEYS if no keys, GETSLOT_CROSSSLOT if cross slot, otherwise the slot number. */ | ||
| * INVALID_CLUSTER_SLOT if no keys, CLUSTER_CROSSSLOT if cross slot, otherwise the slot number. */ | ||
| int getSlotFromCommand(struct redisCommand *cmd, robj **argv, int argc) { | ||
| int slot = GETSLOT_NOKEYS; | ||
| if (!cmd || !server.cluster_enabled) return slot; | ||
| if (!cmd || !server.cluster_enabled) return INVALID_CLUSTER_SLOT; | ||
|
|
||
| /* Get the keys from the command */ | ||
| getKeysResult result = GETKEYS_RESULT_INIT; | ||
| int numkeys = getKeysFromCommand(cmd, argv, argc, &result); | ||
| keyReference *keyindex = result.keys; | ||
|
|
||
| /* Get slot of each key and check if they are all the same */ | ||
| for (int j = 0; j < numkeys; j++) { | ||
| robj *thiskey = argv[keyindex[j].pos]; | ||
| int thisslot = keyHashSlot((char*)thiskey->ptr, sdslen(thiskey->ptr)); | ||
| if (slot == GETSLOT_NOKEYS) { | ||
| slot = thisslot; | ||
| } else if (slot != thisslot) { | ||
| slot = GETSLOT_CROSSSLOT; /* Mark as cross slot */ | ||
| break; | ||
| } | ||
| } | ||
| getKeysFromCommand(cmd, argv, argc, &result); | ||
|
|
||
| /* Extract slot from the keys result. */ | ||
| int slot = extractSlotFromKeysResult(argv, &result); | ||
| getKeysFreeResult(&result); |
There was a problem hiding this comment.
Fix off-by-one in extractSlotFromKeysResult before relying on it here.
getSlotFromCommand now routes every slot calculation through extractSlotFromKeysResult. In src/cluster.c (lines 1102‑1125 in the provided snippet), the loop iterates with j <= keys_result->numkeys while the buffer is sized to numkeys, which can read/write past the end. This is a memory‑safety bug that becomes more exposed with this refactor. Please switch to < before enabling the wider use.
🐛 Suggested fix in src/cluster.c
- for (int j = 0; j <= keys_result->numkeys; j++) {
+ for (int j = 0; j < keys_result->numkeys; j++) {🤖 Prompt for AI Agents
In `@src/db.c` around lines 428 - 439, The loop in extractSlotFromKeysResult (the
code iterating with j <= keys_result->numkeys over the keys_result buffer) is
off-by-one and can read/write past the allocated buffer; update that loop
condition to use j < keys_result->numkeys (and any equivalent <= uses) so you
only iterate up to numkeys-1, and verify any buffers sized with numkeys are
accessed with the corrected index bounds; after fixing
extractSlotFromKeysResult, ensure getSlotFromCommand continues to call
extractSlotFromKeysResult unchanged.
2 participants
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.