Fix free - #1
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses several memory management and buffer handling issues in the codebase. The main focus is on fixing potential buffer overflows, unsafe string operations, and improving memory safety through the use of C++ standard library containers.
Key changes include:
- Fixed buffer overflow issues by replacing
strlen()calls on uninitialized buffers with proper buffer size constants - Converted manual memory management with C-style arrays to safer
std::vector<std::string>containers - Added boundary checking for sequence sampling to prevent negative indices and buffer overruns
- Replaced potentially unsafe
strncpycalls withmemcpyusing correctly calculated lengths
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fasta_sampler.cpp | Added boundary checking for position calculations to handle short sequences and prevent negative indices; added zero-initialization for new arrays; fixed sequence duplication to use correct source pointer |
| add_variants.cpp | Converted indel processing from manual C-style string arrays to std::vector<std::string>; fixed map key invalidation issue by erasing entries before realloc; removed buffer overflow assertions in favor of automatic string memory management |
| add_indels.cpp | Added explicit null terminator after indel modifications to ensure string is properly terminated |
| Sampling.cpp | Fixed critical buffer overflow by using LENS constant instead of strlen() on uninitialized buffers; replaced strncpy with memcpy using actual fragment length; added boundary checks and segment length clamping for circular simulation mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| // Selecting genomic start position across the generated contiguous contigs for which to extract | ||
| int chr_idx = -1; | ||
| int posB = 0; int posE = 0; | ||
| int skipread = 0; // Initialize skipread to 0 |
There was a problem hiding this comment.
The skipread variable is initialized once per main loop iteration (at line 165) but is used inside the for loop at line 348 to skip individual fragments. If skipread is set to 1 for one fragment (e.g., at lines 378 or 384), it will remain 1 for all subsequent fragments in the same main loop iteration, causing them to be incorrectly skipped as well.
The initialization should be moved inside the for loop (after line 349) to reset skipread for each fragment independently, or alternatively, skipread should be explicitly reset to 0 at the start of each for loop iteration.
| // Zero-initialize all new arrays | ||
| memset(seqs, 0, sizeof(char*)*nref); | ||
| memset(seqs_names, 0, sizeof(char*)*nref); | ||
| memset(seqs_l, 0, sizeof(int)*nref); | ||
| memset(realnameidx, 0, sizeof(int)*nref); | ||
| memset(pldmap, 0, sizeof(int)*5); |
There was a problem hiding this comment.
[nitpick] The zero-initialization with memset is redundant for these newly allocated arrays. In C++, new already initializes pointers to unspecified values, and these arrays will be immediately populated with values in the subsequent code (lines 829-834 for the first fs->nref elements, and lines 854-859 for the new elements). The zero-initialization doesn't provide any safety benefit here and adds unnecessary overhead.
| // Zero-initialize all new arrays | |
| memset(seqs, 0, sizeof(char*)*nref); | |
| memset(seqs_names, 0, sizeof(char*)*nref); | |
| memset(seqs_l, 0, sizeof(int)*nref); | |
| memset(realnameidx, 0, sizeof(int)*nref); | |
| memset(pldmap, 0, sizeof(int)*5); | |
| // Zero-initialization of new arrays is unnecessary; arrays will be fully populated below. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| // Selecting genomic start position across the generated contiguous contigs for which to extract | ||
| int chr_idx = -1; | ||
| int posB = 0; int posE = 0; | ||
| int skipread = 0; // Initialize skipread to 0 |
There was a problem hiding this comment.
The variable skipread is initialized here at line 165 but then reset again at line 349 within a loop. This creates redundancy and potential confusion. Consider removing this early initialization since line 349 properly initializes it at the start of each fragment iteration where it's actually used.
| int skipread = 0; // Initialize skipread to 0 |
| memset(FragmentSequence,0,LENS); | ||
| // Copy first segment - end of the chromosome | ||
| strncpy(FragmentSequence, chrseq + segment1_start, segment1_length); | ||
| memcpy(FragmentSequence, chrseq + segment1_start, segment1_length); |
There was a problem hiding this comment.
The memcpy call on line 194 should not include a null terminator since segment1_length is derived from position arithmetic and may not account for it. However, line 196 immediately follows by copying segment2_length bytes starting at segment1_length offset. If segment1_length included space for a null terminator, this would overwrite it. The current code appears correct, but since line 236 adds a null terminator after all copying is complete, verify that fragmentLength calculation at line 190 doesn't need adjustment if the buffer isn't properly sized.
| fs->seqs_names[i] = (char*) realloc(fs->seqs_names[i], (new_length + 1) * sizeof(char)); | ||
| char *old_name = fs->seqs_names[i]; | ||
| fs->char2idx.erase(old_name); | ||
| fs->seqs_names[i] = (char*) realloc(old_name, (new_length + 1) * sizeof(char)); |
There was a problem hiding this comment.
After realloc() on line 616, if the reallocation moves the memory block to a new address, old_name becomes a dangling pointer. However, line 615 erases using old_name before the realloc, which is correct. But if realloc fails and returns NULL, fs->seqs_names[i] will be set to NULL, and the original memory in old_name will be leaked. Add a null check: char *new_name = (char*) realloc(old_name, (new_length + 1) * sizeof(char)); if(!new_name) { /* handle error */ } fs->seqs_names[i] = new_name;
| fs->seqs_names[i] = (char*) realloc(old_name, (new_length + 1) * sizeof(char)); | |
| char *new_name = (char*) realloc(old_name, (new_length + 1) * sizeof(char)); | |
| if (!new_name) { | |
| fprintf(stderr, "Error: realloc failed for sequence name update (index %d)\n", i); | |
| // Optionally, free resources here if needed | |
| return -1; | |
| } | |
| fs->seqs_names[i] = new_name; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
No description provided.