Skip to content

Clean up lookahead-related code#25

Open
tomerqodo wants to merge 7 commits intoqodo_action_req_1_base_clean_up_lookahead-related_code_pr3from
qodo_action_req_1_head_clean_up_lookahead-related_code_pr3
Open

Clean up lookahead-related code#25
tomerqodo wants to merge 7 commits intoqodo_action_req_1_base_clean_up_lookahead-related_code_pr3from
qodo_action_req_1_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 (2) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Off-by-one loop in slot scan 📘 Rule violation ⛯ Reliability
Description
extractSlotFromKeysResult() iterates with j <= keys_result->numkeys, which reads
  keys_result->keys[numkeys] and writes slot_buffer[numkeys] out of bounds.
• This is a missing boundary/edge-case handling issue that can lead to crashes or memory corruption
  under normal inputs.
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 values and failure points. The loop
condition uses <= instead of <, creating an out-of-bounds access at the boundary j == numkeys.

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()` has an off-by-one loop (`&lt;=`) that can access past the end of `keys_result-&gt;keys` and the `slot_buffer`.

## Issue Context
This is a boundary-value bug (j==numkeys) that can cause memory corruption.

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

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


2. *slot not set for zero keys 📘 Rule violation ⛯ Reliability
Description
extractKeysAndSlot() documents that the slot is returned via *slot, but it only assigns
  *slot when num_keys > 0.
• When num_keys == 0 (or negative), *slot may retain a stale value from the caller, causing
  incorrect behavior in downstream logic that relies on *slot being valid/initialized.
Code

src/db.c[R3201-3203]

+    if (num_keys > 0) {
        *slot = extractSlotFromKeysResult(argv, result);
-
+    }
Evidence
PR Compliance ID 3 requires handling empty/boundary cases explicitly. Here, the empty-keys case
(num_keys == 0) is not handled by initializing *slot, despite the function contract stating the
slot is returned via *slot.

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

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()` may leave `*slot` uninitialized/stale when `num_keys` is 0 or negative.

## Issue Context
The function contract says the slot is returned via `*slot`, so callers may rely on it being set even for the no-keys path.

## Fix Focus Areas
- src/db.c[3176-3204]

ⓘ 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 +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

1. Off-by-one loop in slot scan 📘 Rule violation ⛯ Reliability

extractSlotFromKeysResult() iterates with j <= keys_result->numkeys, which reads
  keys_result->keys[numkeys] and writes slot_buffer[numkeys] out of bounds.
• This is a missing boundary/edge-case handling issue that can lead to crashes or memory corruption
  under normal inputs.
Agent prompt
## Issue description
`extractSlotFromKeysResult()` has an off-by-one loop (`<=`) that can access past the end of `keys_result->keys` and the `slot_buffer`.

## Issue Context
This is a boundary-value bug (j==numkeys) that can cause memory corruption.

## 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

2. *slot not set for zero keys 📘 Rule violation ⛯ Reliability

extractKeysAndSlot() documents that the slot is returned via *slot, but it only assigns
  *slot when num_keys > 0.
• When num_keys == 0 (or negative), *slot may retain a stale value from the caller, causing
  incorrect behavior in downstream logic that relies on *slot being valid/initialized.
Agent prompt
## Issue description
`extractKeysAndSlot()` may leave `*slot` uninitialized/stale when `num_keys` is 0 or negative.

## Issue Context
The function contract says the slot is returned via `*slot`, so callers may rely on it being set even for the no-keys path.

## Fix Focus Areas
- src/db.c[3176-3204]

ⓘ 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