Clean up lookahead-related code#16
Conversation
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
WalkthroughThis pull request refactors cluster slot detection mechanisms by introducing a new Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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
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
🤖 Fix all issues with AI agents
In `@src/cluster.c`:
- Around line 1098-1125: The function extractSlotFromKeysResult contains an
off‑by‑one loop (it uses j <= keys_result->numkeys) and allocates an unused
slot_buffer via malloc/free; remove the allocation and all uses of slot_buffer,
change the loop to for (int j = 0; j < keys_result->numkeys; j++), compute
this_slot with keyHashSlot using argv[keys_result->keys[j].pos], compare to
first_slot as currently done, return CLUSTER_CROSSSLOT on mismatch, and return
first_slot at the end — this eliminates the out‑of‑bounds access and the
deprecated malloc/free usage.
In `@src/db.c`:
- Around line 437-438: The loop in extractSlotFromKeysResult is off-by-one and
reads/writes past the allocated slot_buffer and keys_result->keys; change the
loop condition from using <= keys_result->numkeys to < keys_result->numkeys so
the loop iterates indices 0..numkeys-1 (ensuring accesses to
keys_result->keys[j] and slot_buffer[j] stay in-bounds), then run tests to
confirm no more OOB accesses involving slot_buffer, keys_result, or numkeys.
🧹 Nitpick comments (1)
src/db.c (1)
3200-3205: Consider initializing*slotwhennum_keys <= 0for defensive safety.When
num_keysis 0 or -1 (error case),*slotis never assigned, leaving it with whatever value the caller passed in (potentially uninitialized or stale). While callers should checknum_keysbefore using*slot, setting it toINVALID_CLUSTER_SLOTin the else branch would be safer:♻️ Proposed fix
if (num_keys > 0) { *slot = extractSlotFromKeysResult(argv, result); + } else { + *slot = INVALID_CLUSTER_SLOT; } return num_keys;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/cluster.csrc/cluster.hsrc/cluster_asm.csrc/cluster_slot_stats.csrc/db.csrc/networking.csrc/server.csrc/server.h
🧰 Additional context used
🧬 Code graph analysis (3)
src/cluster_slot_stats.c (1)
src/server.h (1)
clusterSlotStatsEnabled(3741-3741)
src/cluster.c (1)
src/cluster.h (1)
keyHashSlot(66-85)
src/db.c (1)
src/cluster.c (1)
extractSlotFromKeysResult(1103-1126)
🪛 GitHub Actions: CI
src/server.h
[error] 4308-4309: Deprecated declarations: 'free' and 'malloc' are marked deprecated in this header.
src/cluster.c
[error] 1110-1124: cluster.c: 'malloc' and 'free' are deprecated. The build enables -Werror, causing these deprecated-declarations to fail compilation.
🪛 GitHub Actions: Codecov
src/cluster.c
[error] 1110-1124: Compilation failed due to deprecated memory management functions: malloc and free are deprecated under -Werror. The build flags promote deprecated-declarations as errors. Adjust code to use appropriate allocation patterns or remove the deprecation (e.g., use realloc or project-specific allocators) to fix the cluster.o compilation errors.
🪛 GitHub Actions: External Server Tests
src/cluster.c
[error] 1110-1124: Werror: deprecated usage of malloc/free detected. Replacement with calloc/realloc or updated memory management required. References to malloc at 1110 and free at 1120/1124 trigger failures.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: Analyze (cpp)
🔇 Additional comments (8)
src/networking.c (1)
5099-5099: Good documentation improvement.The added comment clarifies a non-obvious case where
argv[j]can be NULL due to the partial cleanup performed inreclaimPendingCommandwhen handling IO thread commands. This improves code maintainability by documenting the cross-function dependency.src/cluster.h (1)
26-26: Macro addition looks good.Clear sentinel definition alongside INVALID_CLUSTER_SLOT is consistent with the refactor.
src/server.h (1)
2412-2412: Comment tweak is fine.No functional changes here.
src/cluster_slot_stats.c (1)
139-141: No issue found. The guard functions are correct as written.
c->slot != INVALID_CLUSTER_SLOTproperly excludes bothINVALID_CLUSTER_SLOT (-1)andCLUSTER_CROSSSLOT (-2). When a command has cross-slot keys,extractSlotFromKeysResult()returnsCLUSTER_CROSSSLOT, butpreprocessCommand()(server.c:4143-4145) converts it toINVALID_CLUSTER_SLOTbeforec->slotis assigned. By the time slot-stats aggregation functions execute,c->slotcan only be a valid slot (≥ 0) orINVALID_CLUSTER_SLOT (-1), making theserverAssert(c->slot >= 0)checks safe.Likely an incorrect or invalid review comment.
src/server.c (1)
4135-4147: No action needed. The code is safe as-is.The contract of
extractKeysAndSlot()(defined in src/db.c:3185–3207) only modifies the slot whennum_keys > 0. For zero-key commands, it returnsnum_keys == 0without touching the slot parameter. Sincepcmd->slotis initialized toINVALID_CLUSTER_SLOTat line 4109, the conditionpcmd->slot == CLUSTER_CROSSSLOTat line 4143 will never be true for zero-key commands, preventing false positives.Likely an incorrect or invalid review comment.
src/cluster_asm.c (2)
754-778: LGTM: keyless and cross-slot handling aligned with new sentinels.The INVALID_CLUSTER_SLOT early return and CLUSTER_CROSSSLOT flagging are consistent with the new slot sentinel semantics.
3455-3465: LGTM: module propagation now correctly respects CLUSTER_CROSSSLOT / INVALID_CLUSTER_SLOT.The updated checks are consistent with the new sentinel values and preserve safety around slot range validation.
src/db.c (1)
428-431: LGTM!The updated comment clearly documents the return value semantics, and the early return correctly handles the edge cases when no slot can be determined.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /* Extract slot number from keys in a keys_result structure and return to caller. | ||
| * Returns INVALID_CLUSTER_SLOT if keys belong to different slots (cross-slot error), | ||
| * or if there are no keys. | ||
| */ | ||
| * Returns: | ||
| * - The slot number if all keys belong to the same slot | ||
| * - INVALID_CLUSTER_SLOT if there are no keys or cluster is disabled | ||
| * - CLUSTER_CROSSSLOT if keys belong to different slots (cross-slot error) */ | ||
| int extractSlotFromKeysResult(robj **argv, getKeysResult *keys_result) { | ||
| if (keys_result->numkeys == 0) | ||
| if (keys_result->numkeys == 0 || !server.cluster_enabled) | ||
| return INVALID_CLUSTER_SLOT; | ||
|
|
||
| if (!server.cluster_enabled) | ||
| return 0; | ||
|
|
||
| int first_slot = INVALID_CLUSTER_SLOT; | ||
| for (int j = 0; j < keys_result->numkeys; j++) { | ||
|
|
||
| /* Allocate temporary buffer for slot tracking */ | ||
| int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys); | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
| free(slot_buffer); | ||
| return first_slot; |
There was a problem hiding this comment.
Fix off‑by‑one and remove deprecated malloc/free.
Line 1112 iterates with <= keys_result->numkeys, which reads/writes one past the end of keys_result->keys (and slot_buffer). Also, malloc/free are deprecated in this codebase and currently fail CI. The buffer isn’t used, so removing it fixes both issues.
🛠️ Proposed fix
- int first_slot = INVALID_CLUSTER_SLOT;
-
- /* Allocate temporary buffer for slot tracking */
- int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);
-
- 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;
+ int first_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));
if (first_slot == INVALID_CLUSTER_SLOT)
first_slot = this_slot;
else if (first_slot != this_slot) {
- free(slot_buffer);
return CLUSTER_CROSSSLOT;
}
}
- free(slot_buffer);
return first_slot;🧰 Tools
🪛 GitHub Actions: CI
[error] 1110-1124: cluster.c: 'malloc' and 'free' are deprecated. The build enables -Werror, causing these deprecated-declarations to fail compilation.
🪛 GitHub Actions: Codecov
[error] 1110-1124: Compilation failed due to deprecated memory management functions: malloc and free are deprecated under -Werror. The build flags promote deprecated-declarations as errors. Adjust code to use appropriate allocation patterns or remove the deprecation (e.g., use realloc or project-specific allocators) to fix the cluster.o compilation errors.
🪛 GitHub Actions: External Server Tests
[error] 1110-1124: Werror: deprecated usage of malloc/free detected. Replacement with calloc/realloc or updated memory management required. References to malloc at 1110 and free at 1120/1124 trigger failures.
🤖 Prompt for AI Agents
In `@src/cluster.c` around lines 1098 - 1125, The function
extractSlotFromKeysResult contains an off‑by‑one loop (it uses j <=
keys_result->numkeys) and allocates an unused slot_buffer via malloc/free;
remove the allocation and all uses of slot_buffer, change the loop to for (int j
= 0; j < keys_result->numkeys; j++), compute this_slot with keyHashSlot using
argv[keys_result->keys[j].pos], compare to first_slot as currently done, return
CLUSTER_CROSSSLOT on mismatch, and return first_slot at the end — this
eliminates the out‑of‑bounds access and the deprecated malloc/free usage.
| /* Extract slot from the keys result. */ | ||
| int slot = extractSlotFromKeysResult(argv, &result); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 30 'int extractSlotFromKeysResult' src/cluster.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1398
Critical: Off-by-one error in extractSlotFromKeysResult causes heap buffer overflow.
The extractSlotFromKeysResult function (defined in src/cluster.c, line 1103) has a critical buffer overflow bug. The function allocates slot_buffer with space for numkeys elements (line 1110), but the loop condition on line 1112 uses j <= keys_result->numkeys instead of j < keys_result->numkeys, causing:
- Out-of-bounds read from
keys_result->keys[numkeys](line 1113) - Out-of-bounds write to
slot_buffer[numkeys](line 1115)
Both occur when the loop accesses index numkeys on an array allocated for indices 0 to numkeys-1. Fix by changing the loop condition to j < keys_result->numkeys.
🤖 Prompt for AI Agents
In `@src/db.c` around lines 437 - 438, The loop in extractSlotFromKeysResult is
off-by-one and reads/writes past the allocated slot_buffer and
keys_result->keys; change the loop condition from using <= keys_result->numkeys
to < keys_result->numkeys so the loop iterates indices 0..numkeys-1 (ensuring
accesses to keys_result->keys[j] and slot_buffer[j] stay in-bounds), then run
tests to confirm no more OOB accesses involving slot_buffer, keys_result, or
numkeys.
2 participants
Benchmark PR from qodo-benchmark#94
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.