Skip to content

Clean up lookahead-related code#3

Open
tomerqodo wants to merge 7 commits into
qodo_full_base_clean_up_lookahead-related_code_pr3from
qodo_full_head_clean_up_lookahead-related_code_pr3
Open

Clean up lookahead-related code#3
tomerqodo wants to merge 7 commits into
qodo_full_base_clean_up_lookahead-related_code_pr3from
qodo_full_head_clean_up_lookahead-related_code_pr3

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#3

@qodo-code-review
Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (3) 📎 Requirement gaps (0)

Grey Divider


Action required

1. slot_buffer uses malloc/free 📘 Rule violation ⛯ Reliability
Description
extractSlotFromKeysResult() allocates slot_buffer with malloc() and releases it with
  free() inside src/, which bypasses Redis memory tracking.
• This violates the requirement to use zmalloc/zfree wrappers for dynamic memory management in
  src/ code paths.
Code

src/cluster.c[R1110-1124]

+    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);
Evidence
PR Compliance ID 8 forbids direct malloc/free usage in src/ and requires zmalloc-family
wrappers. The new code in src/cluster.c introduces malloc() and free() for slot_buffer.

AGENTS.md
src/cluster.c[1109-1124]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/cluster.c` uses `malloc()`/`free()` in `src/`, which violates the Redis requirement to use `zmalloc`-family wrappers for allocation and tracking.

## Issue Context
The function `extractSlotFromKeysResult()` allocates a temporary `slot_buffer` using libc allocation.

## Fix Focus Areas
- src/cluster.c[1109-1124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. j <= numkeys out-of-bounds 📘 Rule violation ✓ Correctness
Description
• The loop iterates with j <= keys_result->numkeys, which executes one extra iteration beyond the
  valid keys_result->keys[] and slot_buffer[] bounds.
• This is a boundary-value handling bug that can cause out-of-bounds reads/writes and crashes when
  processing keys (especially when numkeys is small).
Code

src/cluster.c[R1112-1115]

+    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;
Evidence
PR Compliance ID 3 requires explicit handling of boundary/edge cases. The loop condition uses <=
and then indexes keys_result->keys[j] and slot_buffer[j], which will access index numkeys (one
past the end).

Rule 3: Generic: Robust Error Handling and Edge Case Management
src/cluster.c[1112-1115]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`extractSlotFromKeysResult()` iterates one element past the end of the `keys_result-&gt;keys` array and the `slot_buffer` array due to `j &lt;= keys_result-&gt;numkeys`.

## Issue Context
This is a boundary/edge-case bug that can lead to memory corruption or crashes.

## Fix Focus Areas
- src/cluster.c[1112-1115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. *slot left uninitialized 📘 Rule violation ✓ Correctness
Description
extractKeysAndSlot() only assigns *slot when num_keys > 0, leaving *slot unchanged for
  num_keys == 0 (no keys) and potentially propagating an uninitialized/stale slot value to callers.
• This is missing explicit handling for an important boundary case (zero keys), which can produce
  incorrect behavior downstream.
Code

src/db.c[R3201-3203]

+    if (num_keys > 0) {
        *slot = extractSlotFromKeysResult(argv, result);
-
+    }
Evidence
PR Compliance ID 3 requires explicit handling of empty/boundary values. The updated code does not
set *slot for the no-keys case (num_keys == 0), which can leave callers with an invalid slot
state.

Rule 3: Generic: Robust Error Handling and Edge Case Management
src/db.c[3201-3203]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`extractKeysAndSlot()` does not assign `*slot` when there are zero keys, which can leave the output slot value uninitialized or stale.

## Issue Context
Callers of `extractKeysAndSlot()` may rely on `*slot` being set consistently even when `num_keys == 0`.

## Fix Focus Areas
- src/db.c[3201-3203]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



ⓘ The new review experience is currently in Beta. Learn more

Qodo Logo

Comment thread src/cluster.c
Comment on lines +1110 to +1124
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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. slot_buffer uses malloc/free 📘 Rule violation ⛯ Reliability

extractSlotFromKeysResult() allocates slot_buffer with malloc() and releases it with
  free() inside src/, which bypasses Redis memory tracking.
• This violates the requirement to use zmalloc/zfree wrappers for dynamic memory management in
  src/ code paths.
Agent prompt
## Issue description
`src/cluster.c` uses `malloc()`/`free()` in `src/`, which violates the Redis requirement to use `zmalloc`-family wrappers for allocation and tracking.

## Issue Context
The function `extractSlotFromKeysResult()` allocates a temporary `slot_buffer` using libc allocation.

## Fix Focus Areas
- src/cluster.c[1109-1124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/cluster.c
Comment on lines +1112 to +1115
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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. j <= numkeys out-of-bounds 📘 Rule violation ✓ Correctness

• The loop iterates with j <= keys_result->numkeys, which executes one extra iteration beyond the
  valid keys_result->keys[] and slot_buffer[] bounds.
• This is a boundary-value handling bug that can cause out-of-bounds reads/writes and crashes when
  processing keys (especially when numkeys is small).
Agent prompt
## Issue description
`extractSlotFromKeysResult()` iterates one element past the end of the `keys_result->keys` array and the `slot_buffer` array due to `j <= keys_result->numkeys`.

## Issue Context
This is a boundary/edge-case bug that can lead to memory corruption or crashes.

## Fix Focus Areas
- src/cluster.c[1112-1115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/db.c
Comment on lines +3201 to +3203
if (num_keys > 0) {
*slot = extractSlotFromKeysResult(argv, result);

}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. *slot left uninitialized 📘 Rule violation ✓ Correctness

extractKeysAndSlot() only assigns *slot when num_keys > 0, leaving *slot unchanged for
  num_keys == 0 (no keys) and potentially propagating an uninitialized/stale slot value to callers.
• This is missing explicit handling for an important boundary case (zero keys), which can produce
  incorrect behavior downstream.
Agent prompt
## Issue description
`extractKeysAndSlot()` does not assign `*slot` when there are zero keys, which can leave the output slot value uninitialized or stale.

## Issue Context
Callers of `extractKeysAndSlot()` may rely on `*slot` being set consistently even when `num_keys == 0`.

## Fix Focus Areas
- src/db.c[3201-3203]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants