Skip to content

fix: use csv_config_copy to ensure config is properly duplicated in CSV Reader #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kwanddwo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: use csv_config_copy to ensure config is properly duplicated in CSV Reader #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions csv_reader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ CSVReader* csv_reader_init_with_config(Arena *persistent_arena, Arena *temp_aren

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;
Comment on lines 21 to 24

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

Suggested change
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->cached_header_count = 0;
reader->cached_headers = NULL;
Expand Down Expand Up @@ -90,7 +90,7 @@ CSVReader* csv_reader_init_standalone(CSVConfig *config) {

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;
Comment on lines 91 to 94

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

Suggested change
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.

reader->cached_header_count = 0;
reader->cached_headers = NULL;
Expand Down