4.1.11#53
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds allocation-failure handling and reorders duplicate-record checks in list collation, introduces a null-terminator break in byte-wise data comparison, and bumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/collate.c`:
- Around line 191-195: The code uses last_key and last_data (allocated via
calloc) without checking for NULL before doing pointer arithmetic and memcmp in
the duplicate-detection block that computes new_subkey and compares
rec_size/last_rec_size; add NULL checks immediately after the calloc calls that
allocate last_key and last_data and handle allocation failure (e.g., log an
error and abort processing or fall back to a safe path) so that new_subkey =
(memcmp(rec_key+LDB_KEY_LN, last_key+LDB_KEY_LN, subkey_ln) != 0) and
memcmp(data, last_data, rec_size) are never called when last_key or last_data is
NULL; ensure references to rec_key, last_key, data, last_data, rec_size and
last_rec_size are guarded accordingly in the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
scanoss-qg
left a comment
scanoss-qg
left a comment
There was a problem hiding this comment.
LGTM. Slight Changes. Consider CodeRabbit comments
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/collate.c`:
- Around line 182-189: Unify all error exits in ldb_import_list_variable_records
by introducing a single cleanup label (e.g., cleanup or error_cleanup) and
replacing direct returns on allocation/check failures with goto to that label;
ensure the cleanup block frees buffer, last_key, and last_data (if non-NULL),
logs the error via log_error("Memory allocation failed in
ldb_import_list_variable_records\n") if appropriate, and returns false. Update
every early exit path that currently does free() then return (including the
places around the current checks for buffer/last_key/last_data and other
returns) to jump to the shared cleanup label instead so no leak is possible.
Ensure normal success return bypasses the cleanup or performs any needed final
free before returning true.
- Around line 182-189: In ldb_import_list_variable_records, the error path that
checks if (!buffer || !last_key || !last_data) calls the undefined log_error;
replace that call with the available logger function log_info (preserving the
existing message), i.e. update the log_error("Memory allocation failed in
ldb_import_list_variable_records\n") invocation to use log_info so the code
builds against the logger API; ensure the function name
ldb_import_list_variable_records and the variables buffer, last_key, last_data
are left unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (!buffer || !last_key || !last_data) | ||
| { | ||
| free(buffer); | ||
| free(last_key); | ||
| free(last_data); | ||
| log_error("Memory allocation failed in ldb_import_list_variable_records\n"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Unify all failure exits through one cleanup path to prevent leaks.
On Line 221 and Line 269, returning directly skips frees for buffer, last_key, and last_data. Route all error exits to a shared cleanup label.
♻️ Suggested refactor
bool ldb_import_list_variable_records(struct ldb_collate_data *collate)
{
@@
if (!buffer || !last_key || !last_data)
{
- free(buffer);
- free(last_key);
- free(last_data);
- log_info("Memory allocation failed in ldb_import_list_variable_records\n");
- return false;
+ log_info("Memory allocation failed in ldb_import_list_variable_records\n");
+ goto cleanup_fail;
}
@@
int error =ldb_node_write(out_table, new_sector, last_key, buffer, buffer_ptr, 0);
//abort in case of error
if (error < 0)
- return false;
+ goto cleanup_fail;
@@
int error= ldb_node_write(out_table, new_sector, last_key, buffer, buffer_ptr, 0);
//abort in case of error
if (error < 0)
- return false;
+ goto cleanup_fail;
}
@@
free(buffer);
free(last_key);
free(last_data);
return true;
+
+cleanup_fail:
+ free(buffer);
+ free(last_key);
+ free(last_data);
+ return false;
}Also applies to: 218-221, 266-269
🧰 Tools
🪛 GitHub Actions: build
[warning] 187-187: gcc warning: implicit declaration of function 'log_error'; did you mean 'ldb_error'? [-Wimplicit-function-declaration]
[error] 187-187: Link failed (ld): undefined reference to 'log_error' from 'ldb_import_list_variable_records' during building target 'ldb'. make: *** [Makefile:12: ldb] Error 1
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/collate.c` around lines 182 - 189, Unify all error exits in
ldb_import_list_variable_records by introducing a single cleanup label (e.g.,
cleanup or error_cleanup) and replacing direct returns on allocation/check
failures with goto to that label; ensure the cleanup block frees buffer,
last_key, and last_data (if non-NULL), logs the error via log_error("Memory
allocation failed in ldb_import_list_variable_records\n") if appropriate, and
returns false. Update every early exit path that currently does free() then
return (including the places around the current checks for
buffer/last_key/last_data and other returns) to jump to the shared cleanup label
instead so no leak is possible. Ensure normal success return bypasses the
cleanup or performs any needed final free before returning true.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify logger symbols available in this repository (read-only).
fd -i 'logger.h'
rg -nP --type=c --type=h '\blog_error\s*\('
rg -nP --type=c --type=h '\b(log_info|log_debug|ldb_error)\s*\('Repository: scanoss/ldb
Length of output: 16012
Replace undefined log_error with log_info to fix the build break.
Line 187 calls log_error, which is not declared or defined in this codebase. The logger API only provides log_info and log_debug (declared in src/logger.h). Replace with log_info:
- log_error("Memory allocation failed in ldb_import_list_variable_records\n");
+ log_info("Memory allocation failed in ldb_import_list_variable_records\n");🧰 Tools
🪛 GitHub Actions: build
[warning] 187-187: gcc warning: implicit declaration of function 'log_error'; did you mean 'ldb_error'? [-Wimplicit-function-declaration]
[error] 187-187: Link failed (ld): undefined reference to 'log_error' from 'ldb_import_list_variable_records' during building target 'ldb'. make: *** [Makefile:12: ldb] Error 1
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/collate.c` around lines 182 - 189, In ldb_import_list_variable_records,
the error path that checks if (!buffer || !last_key || !last_data) calls the
undefined log_error; replace that call with the available logger function
log_info (preserving the existing message), i.e. update the log_error("Memory
allocation failed in ldb_import_list_variable_records\n") invocation to use
log_info so the code builds against the logger API; ensure the function name
ldb_import_list_variable_records and the variables buffer, last_key, last_data
are left unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/collate.c (1)
182-189:⚠️ Potential issue | 🟠 MajorRoute all error exits through one cleanup path.
Line 221 and Line 269 return directly on
ldb_node_writefailure and bypass freeingbuffer,last_key, andlast_data. This leaks memory on error paths.♻️ Suggested fix
@@ - if (error < 0) - return false; + if (error < 0) + goto cleanup_fail; @@ - if (error < 0) - return false; + if (error < 0) + goto cleanup_fail; @@ free(buffer); free(last_key); free(last_data); return true; + +cleanup_fail: + free(buffer); + free(last_key); + free(last_data); + return false; }Also applies to: 218-221, 266-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/collate.c` around lines 182 - 189, The function ldb_import_list_variable_records leaks buffer, last_key, and last_data on error paths where ldb_node_write fails; change all early returns (including the ldb_node_write failure branches) to jump to a single cleanup path that frees buffer, last_key, and last_data and returns the appropriate boolean. Specifically, in ldb_import_list_variable_records replace direct returns after ldb_node_write failures with setting a failure flag/retval and goto cleanup, implement a cleanup label that frees buffer, last_key, last_data (and any other allocated locals), and then returns the flag, ensuring the existing initial allocation error branch also uses the same cleanup path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/collate.c`:
- Around line 454-455: The loop currently treats prefix-equal payloads as equal
by breaking when either pointer a or b is at string end; change the logic so the
loop only treats strings as equal when both terminate together. Locate the
comparison loop that uses the variables a and b (currently doing if (!*a || !*b)
break;) and update it to only break on simultaneous termination (e.g., if (!*a
&& !*b) break;) and ensure the subsequent delete/compare flow treats the case
where one string ended and the other did not as a non-equal result so prefix
matches do not remove unintended records.
---
Duplicate comments:
In `@src/collate.c`:
- Around line 182-189: The function ldb_import_list_variable_records leaks
buffer, last_key, and last_data on error paths where ldb_node_write fails;
change all early returns (including the ldb_node_write failure branches) to jump
to a single cleanup path that frees buffer, last_key, and last_data and returns
the appropriate boolean. Specifically, in ldb_import_list_variable_records
replace direct returns after ldb_node_write failures with setting a failure
flag/retval and goto cleanup, implement a cleanup label that frees buffer,
last_key, last_data (and any other allocated locals), and then returns the flag,
ensuring the existing initial allocation error branch also uses the same cleanup
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| /* Check if key is different than the last one */ | ||
| new_subkey = (memcmp(rec_key+LDB_KEY_LN, last_key+LDB_KEY_LN, subkey_ln) != 0); | ||
|
|
||
| /* If record is duplicated (same subkey and same data), skip it */ | ||
| if (!new_subkey && rec_size == last_rec_size && !memcmp(data, last_data, rec_size)) continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the structure and locate the file
git ls-files | grep -E "collate\.(c|h)$"Repository: scanoss/ldb
Length of output: 86
🏁 Script executed:
# Get the file size to understand how to read it
wc -l src/collate.cRepository: scanoss/ldb
Length of output: 72
🏁 Script executed:
# Read lines 150-250 to get full context around lines 200-204 and 230
sed -n '150,250p' src/collate.c | cat -nRepository: scanoss/ldb
Length of output: 3935
🏁 Script executed:
# Read the rest of the function to see line 230 and understand the complete logic
sed -n '100,250p' src/collate.c | cat -nRepository: scanoss/ldb
Length of output: 5611
🏁 Script executed:
# Continue reading to see what happens after the new_subkey block
sed -n '230,280p' src/collate.c | cat -nRepository: scanoss/ldb
Length of output: 1676
🏁 Script executed:
# Check if subkey_ln can be 0
grep -n "subkey_ln" src/collate.c | head -20Repository: scanoss/ldb
Length of output: 1551
🏁 Script executed:
# Check the header to understand table_key_ln and LDB_KEY_LN
cat -n src/ldb/collate.h | head -50Repository: scanoss/ldb
Length of output: 1962
🏁 Script executed:
# Check the definition of LDB_KEY_LN to understand the structure
grep -r "define LDB_KEY_LN" . --include="*.h"Repository: scanoss/ldb
Length of output: 124
🏁 Script executed:
# Look for usage patterns to see if table_key_ln can equal LDB_KEY_LN (which would make subkey_ln == 0)
rg "table_key_ln\s*=" src/collate.cRepository: scanoss/ldb
Length of output: 93
🏁 Script executed:
# Verify if memcmp with length 0 returns 0
python3 << 'EOF'
# Test what memcmp does with length 0
# In C, memcmp(ptr1, ptr2, 0) returns 0
# Let's confirm this is the expected behavior
print("memcmp behavior with length 0:")
print("memcmp(any, any, 0) always returns 0")
print("This means: (0 != 0) evaluates to false")
print()
print("So if subkey_ln == 0:")
print(" new_subkey = (memcmp(..., ..., 0) != 0)")
print(" new_subkey = (0 != 0)")
print(" new_subkey = false")
print()
print("On first iteration with rec_group_size == 0:")
print("The new-group block (lines 131-151) would be SKIPPED")
print("This is problematic because the group structure isn't initialized.")
EOFRepository: scanoss/ldb
Length of output: 438
🏁 Script executed:
# Check the actual code flow to confirm the issue scenario
sed -n '200,260p' src/collate.c | cat -nRepository: scanoss/ldb
Length of output: 2407
🏁 Script executed:
# Let's trace through the exact buffer write operations to confirm the issue
# Check how rec_group_start is used when writing the group size
rg "rec_group_start\s*\+" src/collate.c -A 1 -B 1Repository: scanoss/ldb
Length of output: 540
Guard first-iteration group initialization when computing new_subkey.
At line 201, new_subkey is overwritten by memcmp result. When subkey_ln == 0 (which occurs if table_key_ln == LDB_KEY_LN), memcmp(..., ..., 0) returns 0, making new_subkey = false on the first record. This causes the new-group initialization block (lines 231-251) to be skipped, leaving rec_group_start at its initial value of 0 while buffer_ptr advances. Subsequent group size writes at buffer + rec_group_start + subkey_ln then target incorrect buffer locations, corrupting the output.
🛠️ Suggested fix
- new_subkey = (memcmp(rec_key+LDB_KEY_LN, last_key+LDB_KEY_LN, subkey_ln) != 0);
+ new_subkey = (rec_group_size == 0) ||
+ (memcmp(rec_key + LDB_KEY_LN, last_key + LDB_KEY_LN, subkey_ln) != 0);
2 participants
Summary by CodeRabbit
Bug Fixes
Chores