fix: use csv_config_copy to ensure config is properly duplicated in CSV Reader - #16
fix: use csv_config_copy to ensure config is properly duplicated in CSV Reader#16Kwanddwo wants to merge 1 commit into
Conversation
…SVReader initialization
📝 WalkthroughWalkthroughBoth ChangesConfig ownership fix
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (1)
csv_reader.c (1)
31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeader parsing still uses caller's
config, not the arena copy.The header-loading block continues to reference the original
configparameter (e.g.config->hasHeader, passed intocsv_parse_line_inplace) instead ofreader->config. Functionally equivalent right after the copy, but it undermines the intent of decoupling from the caller-owned pointer within the same function and is inconsistent with the new ownership model established at line 23/93.Also applies to: 101-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@csv_reader.c` around lines 31 - 42, The header-loading path in the CSV reader still uses the caller-owned config instead of the arena-owned copy, so update the header parsing block in the CSV reader flow to consistently reference reader->config rather than the config parameter. In the logic that checks hasHeader and calls csv_parse_line_inplace, use the reader-owned configuration everywhere so the function matches the new ownership model established in the reader initialization and stays decoupled from the caller’s pointer; apply the same fix in the other header-loading block noted in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@csv_reader.c`:
- Around line 91-94: `csv_reader_init_with_arena` is missing a NULL check after
`csv_config_copy`, so a failed config copy can return a fully allocated reader
with `reader->config` unset. Update the initialization flow to detect a NULL
return from `csv_config_copy`, then follow the same cleanup pattern already used
in this function by freeing `persistent_arena`, `temp_arena`, and `reader`
before returning failure. Keep the fix localized around
`csv_reader_init_with_arena` and match the existing sibling error paths for
consistency.
- Around line 21-24: The reader setup in csv_reader creation is storing the
result of csv_config_copy directly into reader->config without checking for
NULL. Add a validation step immediately after the csv_config_copy call in the
csv_reader initialization path, and if it fails, clean up and return an error
instead of returning a reader with a NULL config. Use the csv_config_copy result
and the reader->config field as the key symbols to update.
---
Nitpick comments:
In `@csv_reader.c`:
- Around line 31-42: The header-loading path in the CSV reader still uses the
caller-owned config instead of the arena-owned copy, so update the header
parsing block in the CSV reader flow to consistently reference reader->config
rather than the config parameter. In the logic that checks hasHeader and calls
csv_parse_line_inplace, use the reader-owned configuration everywhere so the
function matches the new ownership model established in the reader
initialization and stays decoupled from the caller’s pointer; apply the same fix
in the other header-loading block noted in the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| reader->persistent_arena = persistent_arena; | ||
| reader->temp_arena = temp_arena; | ||
| reader->config = config; | ||
| reader->config = csv_config_copy(persistent_arena, config); | ||
| reader->headers_loaded = false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing NULL check on csv_config_copy result.
csv_config_copy returns NULL on allocation failure (see csv_config.c:32-42), but the result is stored directly into reader->config without validation. If persistent_arena allocation fails here, reader is still returned to the caller with a NULL config, which will crash later wherever reader->config is dereferenced.
🛡️ Proposed fix
reader->persistent_arena = persistent_arena;
reader->temp_arena = temp_arena;
- reader->config = csv_config_copy(persistent_arena, config);
+ reader->config = csv_config_copy(persistent_arena, config);
+ if (!reader->config) {
+ fclose(reader->file);
+ return NULL;
+ }
reader->headers_loaded = false;📝 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.
| reader->persistent_arena = persistent_arena; | |
| reader->temp_arena = temp_arena; | |
| reader->config = config; | |
| reader->config = csv_config_copy(persistent_arena, config); | |
| reader->headers_loaded = false; | |
| reader->persistent_arena = persistent_arena; | |
| reader->temp_arena = temp_arena; | |
| reader->config = csv_config_copy(persistent_arena, config); | |
| if (!reader->config) { | |
| fclose(reader->file); | |
| return NULL; | |
| } | |
| reader->headers_loaded = false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@csv_reader.c` around lines 21 - 24, The reader setup in csv_reader creation
is storing the result of csv_config_copy directly into reader->config without
checking for NULL. Add a validation step immediately after the csv_config_copy
call in the csv_reader initialization path, and if it fails, clean up and return
an error instead of returning a reader with a NULL config. Use the
csv_config_copy result and the reader->config field as the key symbols to
update.
| reader->persistent_arena = persistent_arena; | ||
| reader->temp_arena = temp_arena; | ||
| reader->config = config; | ||
| reader->config = csv_config_copy(persistent_arena, config); | ||
| reader->headers_loaded = false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing NULL check on csv_config_copy result.
Same issue as csv_reader_init_with_config: if csv_config_copy fails, reader->config is silently NULL and the fully-allocated reader is still returned, risking a later NULL-pointer dereference. Also note existing sibling error paths in this function already free persistent_arena/temp_arena/reader consistently on failure, so this new failure path should follow the same cleanup pattern.
🛡️ Proposed fix
reader->persistent_arena = persistent_arena;
reader->temp_arena = temp_arena;
- reader->config = csv_config_copy(persistent_arena, config);
+ reader->config = csv_config_copy(persistent_arena, config);
+ if (!reader->config) {
+ fclose(reader->file);
+ arena_destroy(persistent_arena);
+ arena_destroy(temp_arena);
+ free(persistent_arena);
+ free(temp_arena);
+ free(reader);
+ return NULL;
+ }
reader->headers_loaded = false;📝 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.
| reader->persistent_arena = persistent_arena; | |
| reader->temp_arena = temp_arena; | |
| reader->config = config; | |
| reader->config = csv_config_copy(persistent_arena, config); | |
| reader->headers_loaded = false; | |
| reader->persistent_arena = persistent_arena; | |
| reader->temp_arena = temp_arena; | |
| reader->config = csv_config_copy(persistent_arena, config); | |
| if (!reader->config) { | |
| fclose(reader->file); | |
| arena_destroy(persistent_arena); | |
| arena_destroy(temp_arena); | |
| free(persistent_arena); | |
| free(temp_arena); | |
| free(reader); | |
| return NULL; | |
| } | |
| reader->headers_loaded = false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@csv_reader.c` around lines 91 - 94, `csv_reader_init_with_arena` is missing a
NULL check after `csv_config_copy`, so a failed config copy can return a fully
allocated reader with `reader->config` unset. Update the initialization flow to
detect a NULL return from `csv_config_copy`, then follow the same cleanup
pattern already used in this function by freeing `persistent_arena`,
`temp_arena`, and `reader` before returning failure. Keep the fix localized
around `csv_reader_init_with_arena` and match the existing sibling error paths
for consistency.
1 participant
csv_reader now copies the config properly into persistent_arena using csv_config_copy instead of copying by pointer
This fixes #10
Summary by CodeRabbit