Fix use after free in neighbors, spring_attachments and cell attack #397
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int j=0; j < all_cells->size(); j++) | ||
| { | ||
| Cell* pC = (*all_cells)[j]; | ||
| if (( pC != this) && pC->phenotype.cell_interactions.pAttackTarget == this) { | ||
| pC->phenotype.cell_interactions.pAttackTarget = NULL; | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop iterates over all_cells without synchronization, and the #pragma omp parallel for creates concurrent writes to pAttackTarget. This can cause race conditions if cells are being deleted or modified concurrently. Consider removing the parallel directive or adding proper synchronization.
| void Cell::remove_self_from_attackers( void ) | ||
| { | ||
| #pragma omp parallel for | ||
| for (int j=0; j < all_cells->size(); j++) | ||
| { | ||
| Cell* pC = (*all_cells)[j]; | ||
| if (( pC != this) && pC->phenotype.cell_interactions.pAttackTarget == this) { | ||
| pC->phenotype.cell_interactions.pAttackTarget = NULL; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This O(n) loop iterates through all cells to clean up attack targets, which can be expensive for large simulations with many cells. Consider using a reverse index (e.g., a set/list of attackers stored on each potential target cell) to make this operation O(k) where k is the number of attacking cells, which is typically much smaller than n.
| for ( int i = 0; i < all_cells->size(); i++ ) | ||
| { | ||
| Cell* pC = (*all_cells)[i]; | ||
| if (pC != this) | ||
| { | ||
| auto SearchResult = std::find( | ||
| pC->state.neighbors.begin(),pC->state.neighbors.end(),this ); | ||
| if ( SearchResult != pC->state.neighbors.end() ) | ||
| { | ||
| std::cout << "Cell " << pC->ID << " still has cell " << this->ID << " as a neighbor!" << std::endl; | ||
| pC->state.neighbors.erase( SearchResult ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates over all_cells without thread safety protections. If cell deletion can occur concurrently (e.g., from multiple threads), this could lead to iterator invalidation or accessing deallocated memory. Consider adding appropriate synchronization or documenting that delete_cell must be called from a single-threaded context.
| for ( int i = 0; i < all_cells->size(); i++ ) | ||
| { | ||
| Cell* pC = (*all_cells)[i]; | ||
| if (pC != this) | ||
| { | ||
| auto SearchResult = std::find( | ||
| pC->state.spring_attachments.begin(),pC->state.spring_attachments.end(),this ); | ||
| if ( SearchResult != pC->state.spring_attachments.end() ) | ||
| { | ||
| std::cout << "Cell " << pC->ID << " still has cell " << this->ID << " as a spring attachment!" << std::endl; | ||
| pC->state.spring_attachments.erase( SearchResult ); | ||
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates over all_cells without thread safety protections. If cell deletion can occur concurrently (e.g., from multiple threads), this could lead to iterator invalidation or accessing deallocated memory. Consider adding appropriate synchronization or documenting that delete_cell must be called from a single-threaded context.
1 participant
I had some crash that I wanted to fix for a while, and finally found the issues:
Here the fixes I'm proposing for now:
Clearly, this can be improved, but it fixes the present issues.