Skip to content

Fix use after free in neighbors, spring_attachments and cell attack #397

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
vincent-noel wants to merge 8 commits into
base: development
Choose a base branch
from
Open

Fix use after free in neighbors, spring_attachments and cell attack #397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9ad3b4d
Added cleanup method to remove cells from attackers pAttackTarget poi…
vincent-noel Dec 2, 2025
5fbcfc2
Added hacky cleanup to fix use after free for neighbors and spring at…
vincent-noel Dec 2, 2025
74e24c5
Fix spring attachment cleanup message
vincent-noel Dec 2, 2025
ca591c7
Fix neighbors cleanup message
vincent-noel Dec 2, 2025
0780ef2
Implement comparison simplification for pAttackTarget
vincent-noel Dec 2, 2025
73acd59
Erase now also within omp critical block
vincent-noel Dec 2, 2025
6093eea
Compare cell pointer and not the index
vincent-noel Dec 2, 2025
b7ccc39
removing omp parallel look for cleanup
vincent-noel Dec 3, 2025
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
49 changes: 49 additions & 0 deletions core/PhysiCell_cell.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,9 @@ void delete_cell( int index )
// released internalized substrates (as of 1.5.x releases)
pDeleteMe->release_internalized_substrates();

// new Dec 2, 2025
pDeleteMe->remove_self_from_attackers();

// performance goal: don't delete in the middle -- very expensive reallocation
// alternative: copy last element to index position, then shrink vector by 1 at the end O(constant)

Expand Down Expand Up @@ -3401,6 +3404,23 @@ void Cell::remove_self_from_all_neighbors( void )
else
{ /* future error message */ }
}

// This is a bit of a ugly hack, we need to find the origin of that bug
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Grammatical error: "a ugly hack" should be "an ugly hack" (use "an" before words starting with a vowel sound).

Suggested change
// This is a bit of a ugly hack, we need to find the origin of that bug
// This is a bit of an ugly hack, we need to find the origin of that bug

Copilot uses AI. Check for mistakes.
for ( int i = 0; i < all_cells->size(); i++ )
{
Comment on lines +3409 to +3410
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Potential issue: This function iterates over all_cells while being called during cell deletion in delete_cell(). If multiple cells are being deleted concurrently, the size of all_cells may change during iteration, potentially causing out-of-bounds access or skipped cells. Consider caching the size before the loop: int n = all_cells->size(); and using for (int i=0; i < n; i++).

Suggested change
for ( int i = 0; i < all_cells->size(); i++ )
{
int n = all_cells->size();
for ( int i = 0; i < n; i++ )

Copilot uses AI. Check for mistakes.
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 );
}
}
}
Comment on lines +3409 to +3422
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.


return;
}
Expand Down Expand Up @@ -3430,9 +3450,38 @@ void Cell::remove_all_spring_attachments( void )
// clear my list
state.spring_attachments.clear();
}

// This is a bit of a ugly hack, we need to find the origin of that bug
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Grammatical error: "a ugly hack" should be "an ugly hack" (use "an" before words starting with a vowel sound).

Suggested change
// This is a bit of a ugly hack, we need to find the origin of that bug
// This is a bit of an ugly hack, we need to find the origin of that bug

Copilot uses AI. Check for mistakes.
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 );
}

}
}
Comment on lines +3455 to +3469
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

return;
}

void Cell::remove_self_from_attackers( void )
{
#pragma omp parallel for
for (int j=0; j < all_cells->size(); j++)
Comment on lines +3476 to +3477
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Potential issue: This function iterates over all_cells while being called during cell deletion in delete_cell(). If multiple cells are being deleted concurrently (e.g., in a parallel simulation step), the size of all_cells may change during iteration, potentially causing out-of-bounds access or skipped cells. Consider caching the size before the loop: int n = all_cells->size(); and using for (int j=0; j < n; j++).

Suggested change
#pragma omp parallel for
for (int j=0; j < all_cells->size(); j++)
int n = all_cells->size();
#pragma omp parallel for
for (int j=0; j < n; j++)

Copilot uses AI. Check for mistakes.
{
Cell* pC = (*all_cells)[j];
if (( pC != this) && pC->phenotype.cell_interactions.pAttackTarget == this) {
pC->phenotype.cell_interactions.pAttackTarget = NULL;
Comment on lines +3480 to +3481
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Race condition: The parallel loop reads pC->phenotype.cell_interactions.pAttackTarget on line 3489 and writes to it on line 3490 without synchronization. If multiple threads attempt to delete cells simultaneously, or if other operations modify pAttackTarget concurrently, this can lead to race conditions.

Consider either:

  1. Adding a critical section around the check and assignment (lines 3489-3490), or
  2. Removing the #pragma omp parallel for directive to ensure thread-safety.
Suggested change
if (( pC != this) && pC->phenotype.cell_interactions.pAttackTarget == this) {
pC->phenotype.cell_interactions.pAttackTarget = NULL;
#pragma omp critical
{
if (( pC != this) && pC->phenotype.cell_interactions.pAttackTarget == this) {
pC->phenotype.cell_interactions.pAttackTarget = NULL;
}

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +3476 to +3483
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

This O(n) search through all cells on every deletion is a performance concern for large simulations. Consider maintaining a reverse index or list of cells that have this cell as their attack target, which would allow O(1) cleanup instead of requiring a full search through all cells.

Copilot uses AI. Check for mistakes.
Comment on lines +3477 to +3483
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
Comment on lines +3474 to +3484
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

void attach_cells( Cell* pCell_1, Cell* pCell_2 )
{
Expand Down
1 change: 1 addition & 0 deletions core/PhysiCell_cell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ class Cell : public Basic_Agent
void attach_cell_as_spring( Cell* pAddMe ); // done
void detach_cell_as_spring( Cell* pRemoveMe ); // done
void remove_all_spring_attachments( void ); // done
void remove_self_from_attackers( void );

// I want to eventually deprecate this, by ensuring that
// critical BioFVM and PhysiCell data elements are synced when they are needed
Expand Down
Loading